fix variable replacements in view search conditions (#6756)

* fix variable replacements in view search conditions

* added reference to internal issue

* add catch test

* Added AQL test

CHANGELOG

@@ -1,6 +1,9 @@
 devel
 -----
 
+* disable in-memory cache for edge and traversal data on agency nodes, as it
+  is not needed there
+
 * fixed internal issue #1983: the Web UI was showing a deletion confirmation
   multiple times.
 
@@ -15,11 +18,16 @@ devel
 v3.4.0-rc.3 (XXXX-XX-XX)
 ------------------------
 
-* disable in-memory cache for edge and traversal data on agency nodes, as it
-  is not needed there
-
 * include forward-ported diagnostic options for debugging LDAP connections
 
+* fix internal issue #3065: fix variable replacements by the AQL query 
+  optimizer in arangosearch view search conditions
+
+  The consequence of the missing replacements was that some queries using view
+  search conditions could have failed with error messages such as
+
+  "missing variable #3 (a) for node #7 (EnumerateViewNode) while planning registers"
+
 * support installation of ArangoDB on Windows into directories with multibyte
   character filenames on Windows platforms that used a non-UTF8-codepage
 

arangod/Aql/Ast.cpp

@@ -3573,7 +3573,7 @@ AstNode const* Ast::resolveConstAttributeAccess(AstNode const* node) {
         auto member = node->getMember(i);
 
         if (member->type == NODE_TYPE_OBJECT_ELEMENT &&
-            member->getString() == attributeName) {
+            StringRef(member->getStringValue(), member->getStringLength()) == attributeName) {
           // found the attribute
           node = member->getMember(0);
           if (which == 0) {

arangod/Aql/OptimizerRules.cpp

@@ -57,6 +57,10 @@
 #include "Transaction/Methods.h"
 #include "VocBase/Methods/Collections.h"
 
+#ifdef USE_IRESEARCH
+#include "IResearch/IResearchViewNode.h"
+#endif
+
 #include <boost/optional.hpp>
 #include <tuple>
 
@@ -1959,9 +1963,10 @@ void arangodb::aql::moveFiltersUpRule(Optimizer* opt,
 class arangodb::aql::RedundantCalculationsReplacer final
     : public WalkerWorker<ExecutionNode> {
  public:
-  explicit RedundantCalculationsReplacer(
+  explicit RedundantCalculationsReplacer(Ast* ast,
       std::unordered_map<VariableId, Variable const*> const& replacements)
-      : _replacements(replacements) {}
+      : _ast(ast), 
+        _replacements(replacements) {}
 
   template <typename T>
   void replaceStartTargetVariables(ExecutionNode* en) {
@@ -1998,6 +2003,38 @@ class arangodb::aql::RedundantCalculationsReplacer final
     }
   }
 
+#ifdef USE_IRESEARCH
+  void replaceInView(ExecutionNode* en) {
+    auto view = ExecutionNode::castTo<arangodb::iresearch::IResearchViewNode*>(en);
+    if (view->filterConditionIsEmpty()) {
+      // nothing to do
+      return;
+    }
+    AstNode const& search = view->filterCondition();
+    std::unordered_set<Variable const*> variables;
+    Ast::getReferencedVariables(&search, variables);
+
+    // check if the search condition uses any of the variables that we want to
+    // replace
+    AstNode* cloned = nullptr;
+    for (auto const& it : variables) {
+      if (_replacements.find(it->id) != _replacements.end()) {
+        if (cloned == nullptr) {
+          // only clone the original search condition once
+          cloned = _ast->clone(&search);
+        }
+        // calculation uses a to-be-replaced variable
+        _ast->replaceVariables(cloned, _replacements);
+      }
+    }
+
+    if (cloned != nullptr) {
+      // exchange the filter condition
+      view->filterCondition(cloned);
+    }
+  }
+#endif
+
   bool before(ExecutionNode* en) override final {
     switch (en->getType()) {
       case EN::ENUMERATE_LIST: {
@@ -2005,6 +2042,13 @@ class arangodb::aql::RedundantCalculationsReplacer final
         break;
       }
 
+#ifdef USE_IRESEARCH
+      case EN::ENUMERATE_IRESEARCH_VIEW: {
+        replaceInView(en);
+        break;
+      }
+#endif
+
       case EN::RETURN: {
         replaceInVariable<ReturnNode>(en);
         break;
@@ -2151,6 +2195,7 @@ class arangodb::aql::RedundantCalculationsReplacer final
   }
 
  private:
+  Ast* _ast;
   std::unordered_map<VariableId, Variable const*> const& _replacements;
 };
 
@@ -2279,7 +2324,7 @@ void arangodb::aql::removeRedundantCalculationsRule(
 
   if (!replacements.empty()) {
     // finally replace the variables
-    RedundantCalculationsReplacer finder(replacements);
+    RedundantCalculationsReplacer finder(plan->getAst(), replacements);
     plan->root()->walk(finder);
   }
 
@@ -2369,7 +2414,7 @@ void arangodb::aql::removeUnnecessaryCalculationsRule(
           replacements.emplace(outvars[0]->id, static_cast<Variable const*>(
                                                    rootNode->getData()));
 
-          RedundantCalculationsReplacer finder(replacements);
+          RedundantCalculationsReplacer finder(plan->getAst(), replacements);
           plan->root()->walk(finder);
           toUnlink.emplace(n);
           continue;
@@ -5977,7 +6022,7 @@ void arangodb::aql::inlineSubqueriesRule(Optimizer* opt,
           std::unordered_map<VariableId, Variable const*> replacements;
           replacements.emplace(listNode->outVariable()->id,
                                returnNode->inVariable());
-          RedundantCalculationsReplacer finder(replacements);
+          RedundantCalculationsReplacer finder(plan->getAst(), replacements);
           plan->root()->walk(finder);
 
           plan->clearVarUsageComputed();

tests/IResearch/IResearchQueryJoin-test.cpp

@@ -1538,6 +1538,42 @@ TEST_CASE("IResearchQueryTestJoin", "[iresearch][iresearch-query]") {
     CHECK(expectedDoc == expectedDocs.end());
   }
 
+  // dedicated to https://github.com/arangodb/planning/issues/3065$
+  // Optimizer rule "inline sub-queries" which doesn't handle views correctly$
+  {
+    std::string const query = "LET fullAccounts = (FOR acc1 IN [1] RETURN { 'key': 'A' }) for a IN fullAccounts for d IN testView SEARCH d.name == a.key return d";
+
+    CHECK(arangodb::tests::assertRules(
+      vocbase, query, {
+        arangodb::aql::OptimizerRule::handleArangoSearchViewsRule,
+        arangodb::aql::OptimizerRule::inlineSubqueriesRule
+      }
+    ));
+
+    std::vector<arangodb::velocypack::Slice> expectedDocs {
+      arangodb::velocypack::Slice(insertedDocsView[0].vpack()),
+    };
+
+    auto queryResult = arangodb::tests::executeQuery(vocbase, query);
+    REQUIRE(TRI_ERROR_NO_ERROR == queryResult.code);
+
+    auto result = queryResult.result->slice();
+    CHECK(result.isArray());
+
+    arangodb::velocypack::ArrayIterator resultIt(result);
+    REQUIRE(expectedDocs.size() == resultIt.size());
+
+    // Check documents
+    auto expectedDoc = expectedDocs.begin();
+    for (;resultIt.valid(); resultIt.next(), ++expectedDoc) {
+      auto const actualDoc = resultIt.value();
+      auto const resolved = actualDoc.resolveExternals();
+
+      CHECK((0 == arangodb::basics::VelocyPackHelper::compare(arangodb::velocypack::Slice(*expectedDoc), resolved, true)));
+    }
+    CHECK(expectedDoc == expectedDocs.end());
+  }
+
   // we don't support scorers as a part of any expression (in sort or filter)
   //
   // FOR i IN 1..5

tests/js/common/aql/aql-view-arangosearch-cluster.js

@@ -448,6 +448,22 @@ function IResearchAqlTestSuite(args) {
       });
     },
 
+    testViewInInnerLoopOptimized : function() {
+      var expected = [];
+      expected.push({ a: "foo", b: "bar", c: 0 });
+      expected.push({ a: "foo", b: "baz", c: 0 });
+
+      var result = db._query("LET outer = (FOR out1 IN UnitTestsCollection FILTER out1.a == 'foo' && out1.c == 0 RETURN out1) FOR a IN outer FOR d IN UnitTestsView SEARCH d.a == a.a && d.c == a.c && d.b == a.b OPTIONS {waitForSync: true} SORT d.b ASC RETURN d").toArray();
+
+    assertEqual(result.length, expected.length);
+      var i = 0;
+      result.forEach(function(res) {
+        var doc = expected[i++];
+        assertEqual(doc.a, res.a);
+        assertEqual(doc.b, res.b);
+        assertEqual(doc.c, res.c);
+      });
+    },
   };
 }
 

tests/js/common/aql/aql-view-arangosearch-noncluster.js

@@ -518,6 +518,23 @@ function iResearchAqlTestSuite () {
         assertEqual(doc.c, res.c);
       });
     },
+
+    testViewInInnerLoopOptimized : function() {
+      var expected = [];
+      expected.push({ a: "foo", b: "bar", c: 0 });
+      expected.push({ a: "foo", b: "baz", c: 0 });
+
+      var result = db._query("LET outer = (FOR out1 IN UnitTestsCollection FILTER out1.a == 'foo' && out1.c == 0 RETURN out1) FOR a IN outer FOR d IN UnitTestsView SEARCH d.a == a.a && d.c == a.c && d.b == a.b OPTIONS {waitForSync: true} SORT d.b ASC RETURN d").toArray();
+
+    assertEqual(result.length, expected.length);
+      var i = 0;
+      result.forEach(function(res) {
+        var doc = expected[i++];
+        assertEqual(doc.a, res.a);
+        assertEqual(doc.b, res.b);
+        assertEqual(doc.c, res.c);
+      });
+    },
   };
 }
 

tests/js/server/aql/aql-optimizer-rule-inline-subqueries.js

@@ -167,10 +167,6 @@ function optimizerRuleTestSuite () {
   };
 }
 
-////////////////////////////////////////////////////////////////////////////////
-/// @brief test suite
-////////////////////////////////////////////////////////////////////////////////
-
 function optimizerRuleCollectionTestSuite () {
   var c = null;
   var cn = "UnitTestsOptimizer";
@@ -244,11 +240,79 @@ function optimizerRuleCollectionTestSuite () {
   };
 }
 
-////////////////////////////////////////////////////////////////////////////////
+function optimizerRuleViewTestSuite () {
-/// @brief executes the test suite
+  let cn = "UnitTestsOptimizer";
-////////////////////////////////////////////////////////////////////////////////
+
+  return {
+
+    setUp : function () {
+      db._dropView(cn + "View");
+      db._drop(cn);
+      db._create(cn);
+      db._createView(cn + "View", "arangosearch", { links: { "UnitTestsOptimizer" : { includeAllFields: true } } });
+    },
+
+    tearDown : function () {
+      db._dropView(cn + "View");
+      db._drop(cn);
+    },
+
+    testVariableReplacementInSearchCondition : function () {
+      let query = "LET sub = (RETURN 1) FOR outer IN sub FOR v IN " + cn + "View SEARCH v.something == outer RETURN v";
+
+      let result = AQL_EXPLAIN(query);
+      assertNotEqual(-1, result.plan.rules.indexOf(ruleName), query);
+      
+      let nodes = helper.removeClusterNodesFromPlan(result.plan.nodes);
+
+      assertEqual("ReturnNode", nodes[nodes.length - 1].type);
+      assertEqual("EnumerateViewNode", nodes[nodes.length - 2].type);
+      
+      let viewNode = nodes[nodes.length - 2];
+      assertEqual(cn + "View", viewNode.view);
+      assertEqual("v", viewNode.outVariable.name);
+      assertEqual("n-ary or", viewNode.condition.type);
+      assertEqual("n-ary and", viewNode.condition.subNodes[0].type);
+      assertEqual("compare ==", viewNode.condition.subNodes[0].subNodes[0].type);
+      assertEqual("attribute access", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].type);
+      assertEqual("something",        viewNode.condition.subNodes[0].subNodes[0].subNodes[0].name);
+      assertEqual("reference",        viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].type);
+      assertEqual("v", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].name);
+      assertEqual("reference", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].type);
+      assertEqual("outer", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].name);
+      assertEqual([], viewNode.sortCondition);
+    },
+    
+    testNoVariableReplacementInSearchCondition : function () {
+      let query = "LET sub = (RETURN 1) FOR outer IN sub FOR v IN " + cn + "View SEARCH v.something == 1 RETURN v";
+
+      let result = AQL_EXPLAIN(query);
+      assertNotEqual(-1, result.plan.rules.indexOf(ruleName), query);
+      
+      let nodes = helper.removeClusterNodesFromPlan(result.plan.nodes);
+
+      assertEqual("ReturnNode", nodes[nodes.length - 1].type);
+      assertEqual("EnumerateViewNode", nodes[nodes.length - 2].type);
+      
+      let viewNode = nodes[nodes.length - 2];
+      assertEqual(cn + "View", viewNode.view);
+      assertEqual("v", viewNode.outVariable.name);
+      assertEqual("n-ary or", viewNode.condition.type);
+      assertEqual("n-ary and", viewNode.condition.subNodes[0].type);
+      assertEqual("compare ==", viewNode.condition.subNodes[0].subNodes[0].type);
+      assertEqual("attribute access", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].type);
+      assertEqual("something",        viewNode.condition.subNodes[0].subNodes[0].subNodes[0].name);
+      assertEqual("reference",        viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].type);
+      assertEqual("v", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].name);
+      assertEqual("value", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].type);
+      assertEqual([], viewNode.sortCondition);
+    },
+
+  };
+}
 
 jsunity.run(optimizerRuleTestSuite);
 jsunity.run(optimizerRuleCollectionTestSuite);
+jsunity.run(optimizerRuleViewTestSuite);
 
 return jsunity.done();