Fix condition finders moving filters past modifications into index nodes (#6327)

* Fixed issue with condition finder pulling filter past modification node into index.

* Fixed issue with traversal condition finder pulling filter past modification node into index.

* fixed jslint

arangod/Aql/ConditionFinder.cpp

@@ -40,32 +40,36 @@ bool ConditionFinder::before(ExecutionNode* en) {
     case EN::REMOTE:
     case EN::SUBQUERY:
     case EN::INDEX:
-    case EN::INSERT:
-    case EN::REMOVE:
-    case EN::REPLACE:
-    case EN::UPDATE:
-    case EN::UPSERT:
     case EN::RETURN:
     case EN::TRAVERSAL:
     case EN::SHORTEST_PATH:
 #ifdef USE_IRESEARCH
     case EN::ENUMERATE_IRESEARCH_VIEW:
 #endif
+    {
       // in these cases we simply ignore the intermediate nodes, note
       // that we have taken care of nodes that could throw exceptions
       // above.
       break;
+    }
 
-    case EN::LIMIT:
+    case EN::INSERT:
-      // LIMIT invalidates the sort expression we already found
+    case EN::REMOVE:
+    case EN::REPLACE:
+    case EN::UPDATE:
+    case EN::UPSERT:
+    case EN::LIMIT: {
+      // LIMIT or modification invalidates the sort expression we already found
       _sorts.clear();
       _filters.clear();
       break;
+    }
 
     case EN::SINGLETON:
-    case EN::NORESULTS:
+    case EN::NORESULTS: {
       // in all these cases we better abort
       return true;
+    }
 
     case EN::FILTER: {
       std::vector<Variable const*> invars(en->getVariablesUsedHere());

arangod/Aql/TraversalConditionFinder.cpp

@@ -503,11 +503,6 @@ bool TraversalConditionFinder::before(ExecutionNode* en) {
     case EN::REMOTE:
     case EN::SUBQUERY:
     case EN::INDEX:
-    case EN::INSERT:
-    case EN::REMOVE:
-    case EN::REPLACE:
-    case EN::UPDATE:
-    case EN::UPSERT:
     case EN::RETURN:
     case EN::SORT:
     case EN::ENUMERATE_COLLECTION:
@@ -516,15 +511,29 @@ bool TraversalConditionFinder::before(ExecutionNode* en) {
 #ifdef USE_IRESEARCH
     case EN::ENUMERATE_IRESEARCH_VIEW:
 #endif
+    {
       // in these cases we simply ignore the intermediate nodes, note
       // that we have taken care of nodes that could throw exceptions
       // above.
       break;
+    }
+
+    case EN::INSERT:
+    case EN::REMOVE:
+    case EN::REPLACE:
+    case EN::UPDATE:
+    case EN::UPSERT: {
+      // modification invalidates the filter expression we already found
+      _condition = std::make_unique<Condition>(_plan->getAst());
+      _filterVariables.clear();
+      break;
+    }
 
     case EN::SINGLETON:
-    case EN::NORESULTS:
+    case EN::NORESULTS: {
       // in all these cases we better abort
       return true;
+    }
 
     case EN::FILTER: {
       std::vector<Variable const*> invars = en->getVariablesUsedHere();

tests/js/server/aql/aql-graph-traverser.js

@@ -2492,7 +2492,63 @@ function complexFilteringSuite () {
         // 1 Filter On D
         assertEqual(stats.filtered, 1);
       }
-    }
+    },
+
+    testModify: function () {
+      var query = `WITH ${vn}
+      FOR v, e, p IN 1..2 OUTBOUND @start @@ecol
+      UPDATE v WITH {updated: true} IN @@vcol
+      FILTER p.vertices[1].left == true
+      SORT v._key
+      RETURN v._key`;
+      var bindVars = {
+        '@ecol': en,
+        '@vcol': vn,
+        start: vertex.A
+      };
+      var cursor = db._query(query, bindVars);
+      assertEqual(cursor.count(), 3);
+      assertEqual(cursor.toArray(), ['B', 'C', 'F']);
+      var stats = cursor.getExtra().stats;
+      require('internal').print(JSON.stringify(stats));
+      assertEqual(stats.writesExecuted, 6);
+      assertEqual(stats.scannedFull, 0);
+      if (isCluster) {
+        // 1 Primary lookup A
+        // 2 Edge Lookups (A)
+        // 2 Primary lookup B,D
+        // 2 Edge Lookups (2 B) (0 D)
+        // 2 Primary Lookups (C, F)
+        if (mmfilesEngine) {
+          assertTrue(stats.scannedIndex <= 13);
+        } else {
+          assertTrue(stats.scannedIndex <= 7);
+        }
+      } else {
+        // 2 Edge Lookups (A)
+        // 2 Primary (B, D) for Filtering
+        // 2 Edge Lookups (B)
+        // All edges are cached
+        // 1 Primary Lookups A -> B (B cached)
+        // 1 Primary Lookups A -> B -> C (A, B cached)
+        // 1 Primary Lookups A -> B -> F (A, B cached)
+        // With traverser-read-cache
+        // assertEqual(stats.scannedIndex, 9);
+
+        // Without traverser-read-cache
+        assertTrue(stats.scannedIndex <= 28);
+        /*
+          if(mmfilesEngine){
+          assertEqual(stats.scannedIndex, 17);
+          } else {
+          assertEqual(stats.scannedIndex, 13);
+          }
+        */
+      }
+      // 1 Filter On D
+      assertEqual(stats.filtered, 3);
+    },
+
   };
 }
 

tests/js/server/aql/aql-optimizer-indexes.js

@@ -3774,7 +3774,99 @@ function optimizerIndexesMultiCollectionTestSuite () {
       assertNotEqual(-1, idx, query); // index used for inner query
       assertEqual("skiplist", plan.nodes[sub].subquery.nodes[idx].indexes[0].type);
       assertEqual(-1, subNodeTypes.indexOf("SortNode"), query); // must not have sort node for inner query
-    }
+    },
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test index usage
+////////////////////////////////////////////////////////////////////////////////
+
+    testPreventMoveFilterPastModify1 : function () {
+      c1.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "skiplist", fields: [ "ref" ] });
+      var query = `
+        FOR i IN ${c1.name()}
+          FOR j IN ${c2.name()}
+            UPDATE j WITH { tick: i } in ${c2.name()}
+            FILTER i.value == 1
+          RETURN [i, NEW]
+      `;
+
+      var plan = AQL_EXPLAIN(query).plan;
+      var nodeTypes = plan.nodes.map(function(node) {
+        return node.type;
+      });
+
+      assertEqual("SingletonNode", nodeTypes[0], query);
+      assertEqual(-1, nodeTypes.indexOf("IndexNode"), query); // no index
+      assertNotEqual(-1, nodeTypes.indexOf("FilterNode"), query); // post filter
+    },
+
+    testPreventMoveFilterPastModify2 : function () {
+      c1.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "skiplist", fields: [ "ref" ] });
+      var query = `
+        FOR i IN ${c1.name()}
+          FOR j IN ${c2.name()}
+            UPDATE j WITH { tick: i } in ${c2.name()}
+            FILTER j.value == 1
+          RETURN [i, NEW]
+      `;
+
+      var plan = AQL_EXPLAIN(query).plan;
+      var nodeTypes = plan.nodes.map(function(node) {
+        return node.type;
+      });
+
+      assertEqual("SingletonNode", nodeTypes[0], query);
+      assertEqual(-1, nodeTypes.indexOf("IndexNode"), query); // no index
+      assertNotEqual(-1, nodeTypes.indexOf("FilterNode"), query); // post filter
+    },
+
+    testPreventMoveSortPastModify1 : function () {
+      c1.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "skiplist", fields: [ "ref" ] });
+      var query = `
+        FOR i IN ${c1.name()}
+          FOR j IN ${c2.name()}
+            UPDATE j WITH { tick: i } in ${c2.name()}
+            SORT i.value
+          RETURN [i, NEW]
+      `;
+
+      var plan = AQL_EXPLAIN(query).plan;
+      var nodeTypes = plan.nodes.map(function(node) {
+        return node.type;
+      });
+
+      assertEqual("SingletonNode", nodeTypes[0], query);
+      assertEqual(-1, nodeTypes.indexOf("IndexNode"), query); // no index
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"), query); // post filter
+    },
+
+    testPreventMoveSortPastModify2 : function () {
+      c1.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "hash", fields: [ "value" ] });
+      c2.ensureIndex({ type: "skiplist", fields: [ "ref" ] });
+      var query = `
+        FOR i IN ${c1.name()}
+          FOR j IN ${c2.name()}
+            UPDATE j WITH { tick: i } in ${c2.name()}
+            SORT NEW.value
+          RETURN [i, NEW]
+      `;
+
+      var plan = AQL_EXPLAIN(query).plan;
+      var nodeTypes = plan.nodes.map(function(node) {
+        return node.type;
+      });
+
+      assertEqual("SingletonNode", nodeTypes[0], query);
+      assertEqual(-1, nodeTypes.indexOf("IndexNode"), query); // no index
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"), query); // post filter
+    },
 
   };
 }