Revert "Fixed applying late materialization with calc node between sort an limit (#10439)" (#10446)

This reverts commit e765137c3e.

arangod/Aql/IResearchViewOptimizerRules.cpp

@@ -293,7 +293,7 @@ void lateDocumentMaterializationArangoSearchRule(arangodb::aql::Optimizer* opt,
     if (arangodb::aql::ExecutionNode::ENUMERATE_IRESEARCH_VIEW == loop->getType()) {
       auto & viewNode = *EN::castTo<IResearchViewNode*>(loop);
       if (viewNode.isLateMaterialized()) {
-        continue; // loop is already optimized
+        continue; //loop is aleady optimized
       }
       ExecutionNode* current = limitNode->getFirstDependency();
       ExecutionNode* sortNode = nullptr;
@@ -301,7 +301,6 @@ void lateDocumentMaterializationArangoSearchRule(arangodb::aql::Optimizer* opt,
       // without document body usage before that node.
       // this node could be appended with materializer
       bool stopSearch = false;
-      bool stickToSortNode = false;
       while (current != loop) {
         switch (current->getType()) {
           case arangodb::aql::ExecutionNode::SORT:
@@ -322,21 +321,15 @@ void lateDocumentMaterializationArangoSearchRule(arangodb::aql::Optimizer* opt,
           default: // make clang happy
             break;
         }
-        if (!stopSearch) {
+        if (sortNode != nullptr) {
           ::arangodb::containers::HashSet<Variable const*> currentUsedVars;
           current->getVariablesUsedHere(currentUsedVars);
           if (currentUsedVars.find(&viewNode.outVariable()) != currentUsedVars.end()) {
-            if (sortNode != nullptr) {
+            // we have a doc body used before selected SortNode. Forget it, let`s look for better sort to use
-              // we have a doc body used before selected SortNode. Forget it, let`s look for better sort to use
+            sortNode = nullptr;
-              sortNode = nullptr;
+            // this limit node affects only closest sort, if this sort is invalid
-              // this limit node affects only closest sort, if this sort is invalid
+            // we need to check other limit node
-              // we need to check other limit node
+            stopSearch = true;
-              stopSearch = true;
-            } else {
-              // we are between limit and sort nodes.
-              // late materialization could still be applied but we must insert MATERIALIZE node after sort not after limit
-              stickToSortNode = true;
-            }
           }
         }
         if (stopSearch) {
@@ -359,7 +352,7 @@ void lateDocumentMaterializationArangoSearchRule(arangodb::aql::Optimizer* opt,
 
         // on cluster we need to materialize node stay close to sort node on db server (to avoid network hop for materialization calls)
         // however on single server we move it to limit node to make materialization as lazy as possible
-        auto materializeDependency = ServerState::instance()->isCoordinator() || stickToSortNode ? sortNode : limitNode;
+        auto materializeDependency = ServerState::instance()->isCoordinator() ? sortNode : limitNode;
         auto* dependencyParent = materializeDependency->getFirstParent();
         TRI_ASSERT(dependencyParent);
         dependencyParent->replaceDependency(materializeDependency, materializeNode);

arangod/Aql/IResearchViewOptimizerRules.h

@@ -36,7 +36,7 @@ class ExecutionPlan;
 
 namespace iresearch {
 
-/// @brief moves document materialization from view nodes to materialize nodes
+/// @brief moves document materialization from view nodes to sort nodes
 void lateDocumentMaterializationArangoSearchRule(arangodb::aql::Optimizer* opt,
                      std::unique_ptr<arangodb::aql::ExecutionPlan> plan,
                      arangodb::aql::OptimizerRule const& rule);

arangod/Aql/IndexNodeOptimizerRules.cpp

@@ -201,7 +201,7 @@ void arangodb::aql::lateDocumentMaterializationRule(arangodb::aql::Optimizer* op
     if (arangodb::aql::ExecutionNode::INDEX == loop->getType()) {
       auto indexNode = EN::castTo<IndexNode*>(loop);
       if (indexNode->isLateMaterialized()) {
-        continue; // loop is already optimized
+        continue; // loop is aleady optimized
       }
       auto current = limitNode->getFirstDependency();
       ExecutionNode* sortNode = nullptr;
@@ -209,7 +209,6 @@ void arangodb::aql::lateDocumentMaterializationRule(arangodb::aql::Optimizer* op
       // without document body usage before that node.
       // this node could be appended with materializer
       bool stopSearch = false;
-      bool stickToSortNode = false;
       std::vector<NodeWithAttrs> nodesToChange;
       TRI_idx_iid_t commonIndexId = 0; // use one index only
       while (current != loop) {
@@ -231,17 +230,9 @@ void arangodb::aql::lateDocumentMaterializationRule(arangodb::aql::Optimizer* op
             } else if (!node.attrs.empty()) {
               if (!attributesMatch(commonIndexId, indexNode, node)) {
                 // the node uses attributes which is not in index
-                if (nullptr == sortNode) {
+                stopSearch = true;
-                  // we are between limit and sort nodes.
-                  // late materialization could still be applied but we must insert MATERIALIZE node after sort not after limit
-                  stickToSortNode = true;
-                } else {
-                  // this limit node affects only closest sort, if this sort is invalid
-                  // we need to check other limit node
-                  stopSearch = true;
-                }
               } else {
-                nodesToChange.emplace_back(std::move(node));
+                nodesToChange.emplace_back(node);
               }
             }
             break;
@@ -255,19 +246,15 @@ void arangodb::aql::lateDocumentMaterializationRule(arangodb::aql::Optimizer* op
           default: // make clang happy
             break;
         }
-#ifdef ARANGODB_ENABLE_MAINTAINER_MODE
+        if (sortNode != nullptr && current->getType() != arangodb::aql::ExecutionNode::CALCULATION) {
-        // Currently only calculation nodes expected to use loop variable and we successfully replaced
-        // all references to loop variable. However if some other node types will begin to use
-        // loop variable assertion below will be triggered and this rule should be updated
-        if (!stopSearch && current->getType() != arangodb::aql::ExecutionNode::CALCULATION) {
           ::arangodb::containers::HashSet<Variable const*> currentUsedVars;
           current->getVariablesUsedHere(currentUsedVars);
           if (currentUsedVars.find(indexNode->outVariable()) != currentUsedVars.end()) {
-            TRI_ASSERT(false);
+            // this limit node affects only closest sort, if this sort is invalid
+            // we need to check other limit node
             stopSearch = true;
           }
         }
-#endif
         if (stopSearch) {
           // we have a doc body used before selected SortNode. Forget it, let`s look for better sort to use
           sortNode = nullptr;
@@ -314,7 +301,7 @@ void arangodb::aql::lateDocumentMaterializationRule(arangodb::aql::Optimizer* op
 
         // on cluster we need to materialize node stay close to sort node on db server (to avoid network hop for materialization calls)
         // however on single server we move it to limit node to make materialization as lazy as possible
-        auto materializeDependency = ServerState::instance()->isCoordinator() || stickToSortNode ? sortNode : limitNode;
+        auto materializeDependency = ServerState::instance()->isCoordinator() ? sortNode : limitNode;
         auto dependencyParent = materializeDependency->getFirstParent();
         TRI_ASSERT(dependencyParent != nullptr);
         dependencyParent->replaceDependency(materializeDependency, materializeNode);

arangod/Aql/IndexNodeOptimizerRules.h

@@ -32,7 +32,7 @@ class Optimizer;
 struct OptimizerRule;
 class ExecutionPlan;
 
-/// @brief moves document materialization from index nodes to materialize nodes
+/// @brief moves document materialization from index nodes to sort nodes
 void lateDocumentMaterializationRule(arangodb::aql::Optimizer* opt,
                      std::unique_ptr<arangodb::aql::ExecutionPlan> plan,
                      arangodb::aql::OptimizerRule const& rule);

tests/js/server/aql/aql-optimizer-rule-late-document-materialization-arangosearch.js

@@ -211,24 +211,6 @@ function lateDocumentMaterializationArangoSearchRuleTestSuite () {
       });
       assertEqual(0, expected.size);
     },
-    testQueryResultsWithBetweenCalc() {
-      let query = "FOR d IN " + svn  + " SEARCH d.value IN [1,2, 11, 12] SORT BM25(d) LET c = NOOPT(CONCAT(d._key, '-C')) LIMIT 10  RETURN c ";
-      let plan = AQL_EXPLAIN(query).plan;
-      if (!isCluster) {
-        assertNotEqual(-1, plan.rules.indexOf(ruleName));
-        let result = AQL_EXECUTE(query);
-        assertEqual(4, result.json.length);
-        let expected = new Set(['c1-C', 'c2-C', 'c_1-C', 'c_2-C']);
-        result.json.forEach(function(doc) {
-          assertTrue(expected.has(doc));
-          expected.delete(doc);
-        });
-        assertEqual(0, expected.size);
-      } else {
-        // on cluster this will not be applied as calculation node will be moved up
-        assertEqual(-1, plan.rules.indexOf(ruleName));
-      }
-    },
     testQueryResultsSkipSome() {
       let query = "FOR d IN " + vn  + " SEARCH PHRASE(d.str, 'cat', 'text_en')  SORT TFIDF(d) DESC LIMIT 4, 1 RETURN d ";
       let plan = AQL_EXPLAIN(query).plan;

tests/js/server/aql/aql-optimizer-rule-late-document-materialization-rocksdb.js

@@ -279,26 +279,6 @@ function lateDocumentMaterializationRuleTestSuite () {
         assertEqual(0, expected.size);
       }
     },
-    testQueryResultsWithBetweenCalc() {
-      for (i = 0; i < numOfCollectionIndexes; ++i) {
-        let query = "FOR d IN " + collectionNames[i] + " FILTER d.obj.a == 'a_val' SORT d.obj.c LET c = CONCAT(NOOPT(d.obj.d), '-C')  LIMIT 10 RETURN c";
-        let plan = AQL_EXPLAIN(query).plan;
-        if (!isCluster) {
-          assertNotEqual(-1, plan.rules.indexOf(ruleName));
-          let result = AQL_EXECUTE(query);
-          assertEqual(2, result.json.length);
-          let expected = new Set(['d_val-C', 'd_val_2-C']);
-          result.json.forEach(function(doc) {
-            assertTrue(expected.has(doc));
-            expected.delete(doc);
-          });
-          assertEqual(0, expected.size);
-        } else {
-          // on cluster this will not be applied as calculation node will be moved up
-          assertEqual(-1, plan.rules.indexOf(ruleName));
-        }
-      }
-    },
     testQueryResultsSkipSome() {
       for (i = 0; i < numOfCollectionIndexes; ++i) {
         let query = "FOR d IN " + collectionNames[i] + " FILTER d.obj.a == 'a_val' SORT d.obj.c DESC LIMIT 1, 1 RETURN d";