From d3392a9050fe33471eb59aa62908ce88c40b67fb Mon Sep 17 00:00:00 2001 From: Jan Date: Mon, 8 Oct 2018 23:31:19 +0200 Subject: [PATCH] Bug fix 3.4/fix variable replacement in view search conditions (#6755) * fix variable replacements in view search conditions * unify CHANGELOG * added reference to internal issue * add catch test * Added AQL test --- CHANGELOG | 10 +++ arangod/Aql/Ast.cpp | 2 +- arangod/Aql/OptimizerRules.cpp | 55 +++++++++++-- tests/IResearch/IResearchQueryJoin-test.cpp | 36 +++++++++ .../aql/aql-view-arangosearch-cluster.js | 16 ++++ .../aql/aql-view-arangosearch-noncluster.js | 17 ++++ .../aql-optimizer-rule-inline-subqueries.js | 78 +++++++++++++++++-- 7 files changed, 201 insertions(+), 13 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 0ae3d0f24f..a929f95885 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,16 @@ v3.4.0-rc.3 (XXXX-XX-XX) ------------------------ +* include forward-ported diagnostic options for debugging LDAP connections + +* fixed 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" + * fixed internal issue #1983: the Web UI was showing a deletion confirmation multiple times. diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index 372f9b65b8..240c01b1a6 100644 --- a/arangod/Aql/Ast.cpp +++ b/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) { diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index be177a3d9b..13dc5a9f81 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/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 #include @@ -1959,9 +1963,10 @@ void arangodb::aql::moveFiltersUpRule(Optimizer* opt, class arangodb::aql::RedundantCalculationsReplacer final : public WalkerWorker { public: - explicit RedundantCalculationsReplacer( + explicit RedundantCalculationsReplacer(Ast* ast, std::unordered_map const& replacements) - : _replacements(replacements) {} + : _ast(ast), + _replacements(replacements) {} template void replaceStartTargetVariables(ExecutionNode* en) { @@ -1998,6 +2003,38 @@ class arangodb::aql::RedundantCalculationsReplacer final } } +#ifdef USE_IRESEARCH + void replaceInView(ExecutionNode* en) { + auto view = ExecutionNode::castTo(en); + if (view->filterConditionIsEmpty()) { + // nothing to do + return; + } + AstNode const& search = view->filterCondition(); + std::unordered_set 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(en); break; @@ -2151,6 +2195,7 @@ class arangodb::aql::RedundantCalculationsReplacer final } private: + Ast* _ast; std::unordered_map 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( 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 replacements; replacements.emplace(listNode->outVariable()->id, returnNode->inVariable()); - RedundantCalculationsReplacer finder(replacements); + RedundantCalculationsReplacer finder(plan->getAst(), replacements); plan->root()->walk(finder); plan->clearVarUsageComputed(); diff --git a/tests/IResearch/IResearchQueryJoin-test.cpp b/tests/IResearch/IResearchQueryJoin-test.cpp index 437263ffe7..5faa594f2d 100644 --- a/tests/IResearch/IResearchQueryJoin-test.cpp +++ b/tests/IResearch/IResearchQueryJoin-test.cpp @@ -1537,6 +1537,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 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 diff --git a/tests/js/common/aql/aql-view-arangosearch-cluster.js b/tests/js/common/aql/aql-view-arangosearch-cluster.js index b1d520a06c..d8872f7fe8 100644 --- a/tests/js/common/aql/aql-view-arangosearch-cluster.js +++ b/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); + }); + }, }; } diff --git a/tests/js/common/aql/aql-view-arangosearch-noncluster.js b/tests/js/common/aql/aql-view-arangosearch-noncluster.js index d2c4d1263e..5ec39d2818 100644 --- a/tests/js/common/aql/aql-view-arangosearch-noncluster.js +++ b/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); + }); + }, }; } diff --git a/tests/js/server/aql/aql-optimizer-rule-inline-subqueries.js b/tests/js/server/aql/aql-optimizer-rule-inline-subqueries.js index 7ea96ebede..39892b392b 100644 --- a/tests/js/server/aql/aql-optimizer-rule-inline-subqueries.js +++ b/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 () { }; } -//////////////////////////////////////////////////////////////////////////////// -/// @brief executes the test suite -//////////////////////////////////////////////////////////////////////////////// +function optimizerRuleViewTestSuite () { + 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();