address review

Author: Zoran Cvetkov

graph/src/blockchain/block_stream.rs

@@ -455,12 +455,12 @@ async fn get_entities_for_range(
     from: BlockNumber,
     to: BlockNumber,
 ) -> Result<BTreeMap<BlockNumber, Vec<EntityWithType>>, Error> {
-    let mut entity_types = vec![];
-    for entity_name in &filter.entities {
-        let entity_type = schema.entity_type(entity_name)?;
-        entity_types.push(entity_type);
-    }
-    Ok(store.get_range(entity_types, CausalityRegion::ONCHAIN, from..to)?)
+    let entity_types: Result<Vec<EntityType>> = filter
+        .entities
+        .iter()
+        .map(|name| schema.entity_type(name))
+        .collect();
+    Ok(store.get_range(entity_types?, CausalityRegion::ONCHAIN, from..to)?)
 }
 
 impl<C: Blockchain> TriggersAdapterWrapper<C> {

store/postgres/src/block_range.rs

@@ -132,30 +132,38 @@ impl<'a> QueryFragment<Pg> for BlockRangeUpperBoundClause<'a> {
     }
 }
 
+#[derive(Debug, Clone, Copy)]
+pub enum BoundSide {
+    Lower,
+    Upper,
+}
+
 /// Helper for generating SQL fragments for selecting entities in a specific block range
 #[derive(Debug, Clone, Copy)]
 pub enum EntityBlockRange {
-    Mutable((BlockRange, bool)),
+    Mutable((BlockRange, BoundSide)),
     Immutable(BlockRange),
 }
 
 impl EntityBlockRange {
     pub fn new(
         immutable: bool,
         block_range: std::ops::Range<BlockNumber>,
-        is_uppper_range: bool,
+        bound_side: BoundSide,
     ) -> Self {
         let start: Bound<BlockNumber> = Bound::Included(block_range.start);
         let end: Bound<BlockNumber> = Bound::Excluded(block_range.end);
         let block_range: BlockRange = BlockRange(start, end);
         if immutable {
             Self::Immutable(block_range)
         } else {
-            Self::Mutable((block_range, is_uppper_range))
+            Self::Mutable((block_range, bound_side))
         }
     }
 
-    /// Output SQL that matches only rows whose block range contains `block`.
+    /// Output SQL that matches only rows whose block or block_range is
+    /// contained in a range of blocks. For instance:
+    /// lower(block_range) >= $1 and lower(block_range) <= $2
     pub fn contains<'b>(&'b self, out: &mut AstPass<'_, 'b, Pg>) -> QueryResult<()> {
         out.unsafe_to_cache_prepared();
         let block_range = match self {
@@ -190,13 +198,10 @@ impl EntityBlockRange {
 
     pub fn compare_column(&self, out: &mut AstPass<Pg>) {
         match self {
-            EntityBlockRange::Mutable((_, is_upper_range)) => {
-                if *is_upper_range {
-                    out.push_sql(" upper(block_range) ")
-                } else {
-                    out.push_sql(" lower(block_range) ")
-                }
-            }
+            EntityBlockRange::Mutable((_, bound_side)) => match bound_side {
+                BoundSide::Lower => out.push_sql(" lower(block_range) "),
+                BoundSide::Upper => out.push_sql(" upper(block_range) "),
+            },
             EntityBlockRange::Immutable(_) => out.push_sql(" block$ "),
         }
     }

store/postgres/src/relational.rs

@@ -71,7 +71,7 @@ use graph::prelude::{
     QueryExecutionError, StoreError, StoreEvent, ValueType, BLOCK_NUMBER_MAX,
 };
 
-use crate::block_range::{BLOCK_COLUMN, BLOCK_RANGE_COLUMN};
+use crate::block_range::{BoundSide, BLOCK_COLUMN, BLOCK_RANGE_COLUMN};
 pub use crate::catalog::Catalog;
 use crate::connection_pool::ForeignServer;
 use crate::{catalog, deployment};
@@ -530,14 +530,27 @@ impl Layout {
             et_map.insert(et.to_string(), Arc::new(et));
         }
         let mut entities: BTreeMap<BlockNumber, Vec<EntityWithType>> = BTreeMap::new();
-        let lower_vec = FindRangeQuery::new(&tables, causality_region, false, block_range.clone())
-            .get_results::<EntityDataExt>(conn)
-            .optional()?
-            .unwrap_or_default();
-        let upper_vec = FindRangeQuery::new(&tables, causality_region, true, block_range)
-            .get_results::<EntityDataExt>(conn)
-            .optional()?
-            .unwrap_or_default();
+
+        // collect all entities that have their 'lower(block_range)' attribute in the
+        // interval of blocks defined by the variable block_range. For the immutable
+        // entities the respective attribute is 'block$'.
+        let lower_vec = FindRangeQuery::new(
+            &tables,
+            causality_region,
+            BoundSide::Lower,
+            block_range.clone(),
+        )
+        .get_results::<EntityDataExt>(conn)
+        .optional()?
+        .unwrap_or_default();
+        // collect all entities that have their 'upper(block_range)' attribute in the
+        // interval of blocks defined by the variable block_range. For the immutable
+        // entities no entries are returned.
+        let upper_vec =
+            FindRangeQuery::new(&tables, causality_region, BoundSide::Upper, block_range)
+                .get_results::<EntityDataExt>(conn)
+                .optional()?
+                .unwrap_or_default();
         let mut lower_iter = lower_vec.iter().fuse().peekable();
         let mut upper_iter = upper_vec.iter().fuse().peekable();
         let mut lower_now = lower_iter.next();
@@ -560,27 +573,36 @@ impl Layout {
             };
             Ok((ewt, block))
         };
+
+        // The algorithm advances simultaneously entities from both lower_vec and upper_vec and tries
+        // to match entities that have entries in both vectors for a particular block. The match is
+        // successfull if an entry in one array has same values for the number of the block, entity
+        // name and the entity id. The comparison operation over the EntityDataExt fullfils that check.
+        // In addition to that, it also helps to order the elements so the algorith can detect if one
+        // side of the range exists and the other is missing. That way a creation and deletion are
+        // deduced. For immutable entities the entries in upper_vec are missing hence they are considered
+        // having a lower bound at particular block and upper bound at infinity.
         while lower_now.is_some() || upper_now.is_some() {
             let (ewt, block) = if lower_now.is_some() {
                 if upper_now.is_some() {
                     if lower > upper {
                         // we have upper bound at this block, but no lower bounds at the same block so it's deletion
                         let (ewt, block) = transform(upper, EntitySubgraphOperation::Delete)?;
-                        // advance upper
+                        // advance upper_vec pointer
                         upper_now = upper_iter.next();
                         upper = upper_now.unwrap_or(&EntityDataExt::default()).clone();
                         (ewt, block)
                     } else if lower < upper {
                         // we have lower bound at this block but no upper bound at the same block so its creation
                         let (ewt, block) = transform(lower, EntitySubgraphOperation::Create)?;
-                        // advance lower
+                        // advance lower_vec pointer
                         lower_now = lower_iter.next();
                         lower = lower_now.unwrap_or(&EntityDataExt::default()).clone();
                         (ewt, block)
                     } else {
                         assert!(upper == lower);
                         let (ewt, block) = transform(lower, EntitySubgraphOperation::Modify)?;
-                        // advance both
+                        // advance both lower_vec and upper_vec pointers
                         lower_now = lower_iter.next();
                         lower = lower_now.unwrap_or(&EntityDataExt::default()).clone();
                         upper_now = upper_iter.next();
@@ -590,7 +612,7 @@ impl Layout {
                 } else {
                     // we have lower bound at this block but no upper bound at the same block so its creation
                     let (ewt, block) = transform(lower, EntitySubgraphOperation::Create)?;
-                    // advance lower
+                    // advance lower_vec pointer
                     lower_now = lower_iter.next();
                     lower = lower_now.unwrap_or(&EntityDataExt::default()).clone();
                     (ewt, block)
@@ -599,7 +621,7 @@ impl Layout {
                 // we have upper bound at this block, but no lower bounds at all so it's deletion
                 assert!(upper_now.is_some());
                 let (ewt, block) = transform(upper, EntitySubgraphOperation::Delete)?;
-                // advance upper
+                // advance upper_vec pointer
                 upper_now = upper_iter.next();
                 upper = upper_now.unwrap_or(&EntityDataExt::default()).clone();
                 (ewt, block)

store/postgres/src/relational_queries.rs

@@ -35,7 +35,7 @@ use std::ops::Range;
 use std::str::FromStr;
 use std::string::ToString;
 
-use crate::block_range::EntityBlockRange;
+use crate::block_range::{BoundSide, EntityBlockRange};
 use crate::relational::{
     Column, ColumnType, Layout, SqlName, Table, BYTE_ARRAY_PREFIX_SIZE, PRIMARY_KEY_COLUMN,
     STRING_PREFIX_SIZE,
@@ -2025,7 +2025,7 @@ impl<'a, Conn> RunQueryDsl<Conn> for FindQuery<'a> {}
 pub struct FindRangeQuery<'a> {
     tables: &'a Vec<&'a Table>,
     causality_region: CausalityRegion,
-    is_upper_range: bool,
+    bound_side: BoundSide,
     imm_range: EntityBlockRange,
     mut_range: EntityBlockRange,
 }
@@ -2034,15 +2034,15 @@ impl<'a> FindRangeQuery<'a> {
     pub fn new(
         tables: &'a Vec<&Table>,
         causality_region: CausalityRegion,
-        is_upper_range: bool,
+        bound_side: BoundSide,
         block_range: Range<BlockNumber>,
     ) -> Self {
-        let imm_range = EntityBlockRange::new(true, block_range.clone(), false);
-        let mut_range = EntityBlockRange::new(false, block_range, is_upper_range);
+        let imm_range = EntityBlockRange::new(true, block_range.clone(), bound_side);
+        let mut_range = EntityBlockRange::new(false, block_range, bound_side);
         Self {
             tables,
             causality_region,
-            is_upper_range,
+            bound_side,
             imm_range,
             mut_range,
         }
@@ -2056,7 +2056,7 @@ impl<'a> QueryFragment<Pg> for FindRangeQuery<'a> {
 
         for table in self.tables.iter() {
             // the immutable entities don't have upper range and also can't be modified or deleted
-            if !(self.is_upper_range && table.immutable) {
+            if matches!(self.bound_side, BoundSide::Lower) || !table.immutable {
                 if first {
                     first = false;
                 } else {