8000 fix variable replacements in view search conditions (#6756) · davinash/arangodb@7b5d163 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7b5d163

Browse files
jsteemannAndrey Abramov
jsteemann
authored and
Andrey Abramov
committed
fix variable replacements in view search conditions (arangodb#6756)
* fix variable replacements in view search conditions * added reference to internal issue * add catch test * Added AQL test
1 parent 82bb80a commit 7b5d163

File tree

7 files changed

+202
-16
lines changed

7 files changed

+202
-16
lines changed

‎CHANGELOG

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
devel
22
-----
33

4+
* disable in-memory cache for edge and traversal data on agency nodes, as it
5+
is not needed there
6+
47
* fixed internal issue #1983: the Web UI was showing a deletion confirmation
58
multiple times.
69

@@ -15,11 +18,16 @@ devel
1518
v3.4.0-rc.3 (XXXX-XX-XX)
1619
------------------------
1720

18-
* disable in-memory cache for edge and traversal data on agency nodes, as it
19-
is not needed there
20-
2121
* include forward-ported diagnostic options for debugging LDAP connections
2222

23+
* fix internal issue #3065: fix variable replacements by the AQL query
24+
optimizer in arangosearch view search conditions
25+
26+
The consequence of the missing replacements was that some queries using view
27+
search conditions could have failed with error messages such as
28+
29+
"missing variable #3 (a) for node #7 (EnumerateViewNode) while planning registers"
30+
2331
* support installation of ArangoDB on Windows into directories with multibyte
2432
character filenames on Windows platforms that used a non-UTF8-codepage
2533

‎arangod/Aql/Ast.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3573,7 +3573,7 @@ AstNode const* Ast::resolveConstAttributeAccess(AstNode const* node) {
35733573
auto member = node->getMember(i);
35743574

35753575
if (member->type == NODE_TYPE_OBJECT_ELEMENT &&
3576-
member->getString() == attributeName) {
3576+
StringRef(member->getStringValue(), member->getStringLength()) == attributeName) {
35773577
// found the attribute
35783578
node = member->getMember(0);
35793579
if (which == 0) {

‎arangod/Aql/OptimizerRules.cpp

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@
5757
#include "Transaction/Methods.h"
5858
#include "VocBase/Methods/Collections.h"
5959

60+
#ifdef USE_IRESEARCH
61+
#include "IResearch/IResearchViewNode.h"
62+
#endif
63+
6064
#include <boost/optional.hpp>
6165
#include <tuple>
6266

@@ -1959,9 +1963,10 @@ void arangodb::aql::moveFiltersUpRule(Optimizer* opt,
19591963
class arangodb::aql::RedundantCalculationsReplacer final
19601964
: public WalkerWorker<ExecutionNode> {
19611965
public:
1962-
explicit RedundantCalculationsReplacer(
1966+
explicit RedundantCalculationsReplacer(Ast* ast,
19631967
std::unordered_map<VariableId, Variable const*> const& replacements)
1964-
: _replacements(replacements) {}
1968+
: _ast(ast),
1969+
_replacements(replacements) {}
19651970

19661971
template <typename T>
19671972
void replaceStartTargetVariables(ExecutionNode* en) {
@@ -1998,13 +2003,52 @@ class arangodb::aql::RedundantCalculationsReplacer final
19982003
}
19992004
}
20002005

2006+
#ifdef USE_IRESEARCH
2007+
void replaceInView(ExecutionNode* en) {
2008+
auto view = ExecutionNode::castTo<arangodb::iresearch::IResearchViewNode*>(en);
2009+
if (view->filterConditionIsEmpty()) {
2010+
// nothing to do
2011+
return;
2012+
}
2013+
AstNode const& search = view->filterCondition();
2014+
std::unordered_set<Variable const*> variables;
2015+
Ast::getReferencedVariables(&search, variables);
2016+
2017+
// check if the search condition uses any of the variables that we want to
2018+
// replace
2019+
AstNode* cloned = nullptr;
2020+
for (auto const& it : variables) {
2021+
if (_replacements.find(it->id) != _replacements.end()) {
2022+
if (cloned == nullptr) {
2023+
// only clone the original search condition once
2024+
cloned = _ast->clone(&search);
2025+
}
2026+
// calculation uses a to-be-replaced variable
2027+
_ast->replaceVariables(cloned, _replacements);
2028+
}
2029+
}
2030+
2031+
if (cloned != nullptr) {
2032+
// exchange the filter condition
2033+
view->filterCondition(cloned);
2034+
}
2035+
}
2036+
#endif
2037+
20012038
bool before(ExecutionNode* en) override final {
20022039
switch (en->getType()) {
20032040
case EN::ENUMERATE_LIST: {
20042041
replaceInVariable<EnumerateListNode>(en);
20052042
break;
20062043
}
20072044

2045+
#ifdef USE_IRESEARCH
2046+
case EN::ENUMERATE_IRESEARCH_VIEW: {
2047+
replaceInView(en);
2048+
break;
2049+
}
2050+
#endif
2051+
20082052
case EN::RETURN: {
20092053
replaceInVariable<ReturnNode>(en);
20102054
break;
@@ -2151,6 +2195,7 @@ class arangodb::aql::RedundantCalculationsReplacer final
21512195
}
21522196

21532197
private:
2198+
Ast* _ast;
21542199
std::unordered_map<VariableId, Variable const*> const& _replacements;
21552200
};
21562201

@@ -2279,7 +2324,7 @@ void arangodb::aql::removeRedundantCalculationsRule(
22792324

22802325
if (!replacements.empty()) {
22812326
// finally replace the variables
2282-
RedundantCalculationsReplacer finder(replacements);
2327+
RedundantCalculationsReplacer finder(plan->getAst(), replacements);
22832328
plan->root()->walk(finder);
22842329
}
22852330

@@ -2369,7 +2414,7 @@ void arangodb::aql::removeUnnecessaryCalculationsRule(
23692414
replacements.emplace(outvars[0]->id, static_cast<Variable const*>(
23702415
rootNode->getData()));
23712416

2372-
RedundantCalculationsReplacer finder(replacements);
2417+
RedundantCalculationsReplacer finder(plan->getAst(), replacements);
23732418
plan->root()->walk(finder);
23742419
toUnlink.emplace(n);
23752420
continue;
@@ -5977,7 +6022,7 @@ void arangodb::aql::inlineSubqueriesRule(Optimizer* opt,
59776022
std::unordered_map<VariableId, Variable const*> replacements;
59786023
replacements.emplace(listNode->outVariable()->id,
59796024
returnNode->inVariable());
5980-
RedundantCalculationsReplacer finder(replacements);
6025+
RedundantCalculationsReplacer finder(plan->getAst(), replacements);
59816026
plan->root()->walk(finder);
59826027

59836028
plan->clearVarUsageComputed();

‎tests/IResearch/IResearchQueryJoin-test.cpp

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1538,6 +1538,42 @@ TEST_CASE("IResearchQueryTestJoin", "[iresearch][iresearch-query]") {
15381538
CHECK(expectedDoc == expectedDocs.end());
15391539
}
15401540

1541+
// dedicated to https://github.com/arangodb/planning/issues/3065$
1542+
// Optimizer rule "inline sub-queries" which doesn't handle views correctly$
1543+
{
1544+
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";
1545+
1546+
CHECK(arangodb::tests::assertRules(
1547+
vocbase, query, {
1548+
arangodb::aql::OptimizerRule::handleArangoSearchViewsRule,
1549+
arangodb::aql::OptimizerRule::inlineSubqueriesRule
1550+
}
1551+
));
1552+
1553+
std::vector<arangodb::velocypack::Slice> expectedDocs {
1554+
arangodb::velocypack::Slice(insertedDocsView[0].vpack()),
1555+
};
1556+
1557+
auto queryResult = arangodb::tests::executeQuery(vocbase, query);
1558+
REQUIRE(TRI_ERROR_NO_ERROR == queryResult.code);
1559+
1560+
auto result = queryResult.result->slice();
1561+
CHECK(result.isArray());
1562+
1563+
arangodb::velocypack::ArrayIterator resultIt(result);
1564+
REQUIRE(expectedDocs.size() == resultIt.size());
1565+
1566+
// Check documents
1567+
auto expectedDoc = expectedDocs.begin();
1568+
for (;resultIt.valid(); resultIt.next(), ++expectedDoc) {
1569+
auto const actualDoc = resultIt.value();
1570+
auto const resolved = actualDoc.resolveExternals();
1571+
1572+
CHECK((0 == arangodb::basics::VelocyPackHelper::compare(arangodb::velocypack::Slice(*expectedDoc), resolved, true)));
1573+
}
1574+
CHECK(expectedDoc == expectedDocs.end());
1575+
}
1576+
15411577
// we don't support scorers as a part of any expression (in sort or filter)
15421578
//
15431579
// FOR i IN 1..5

‎tests/js/com 10000 mon/aql/aql-view-arangosearch-cluster.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,22 @@ function IResearchAqlTestSuite(args) {
448448
});
449449
},
450450

451+
testViewInInnerLoopOptimized : function() {
452+
var expected = [];
453+
expected.push({ a: "foo", b: "bar", c: 0 });
454+
expected.push({ a: "foo", b: "baz", c: 0 });
455+
456+
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();
457+
458+
assertEqual(result.length, expected.length);
459+
var i = 0;
460+
result.forEach(function(res) {
461+
var doc = expected[i++];
462+
assertEqual(doc.a, res.a);
463+
assertEqual(doc.b, res.b);
464+
assertEqual(doc.c, res.c);
465+
});
466+
},
451467
};
452468
}
453469

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,23 @@ function iResearchAqlTestSuite () {
518518
assertEqual(doc.c, res.c);
519519
});
520520
},
521+
522+
testViewInInnerLoopOptimized : function() {
523+
var expected = [];
524+
expected.push({ a: "foo", b: "bar", c: 0 });
525+
expected.push({ a: "foo", b: "baz", c: 0 });
526+
527+
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();
528+
529+
assertEqual(result.length, expected.length);
530+
var i = 0;
531+
result.forEach(function(res) {
532+
var doc = expected[i++];
533+
assertEqual(doc.a, res.a);
534+
assertEqual(doc.b, res.b);
535+
assertEqual(doc.c, res.c);
536+
});
537+
},
521538
};
522539
}
523540

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

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,6 @@ function optimizerRuleTestSuite () {
167167
};
168168
}
169169

170-
////////////////////////////////////////////////////////////////////////////////
171-
/// @brief test suite
172-
////////////////////////////////////////////////////////////////////////////////
173-
174170
function optimizerRuleCollectionTestSuite () {
175171
var c = null;
176172
var cn = "UnitTestsOptimizer";
@@ -244,11 +240,79 @@ function optimizerRuleCollectionTestSuite () {
244240
};
245241
}
246242

247-
////////////////////////////////////////////////////////////////////////////////
248-
/// @brief executes the test suite
249-
////////////////////////////////////////////////////////////////////////////////
243+
function optimizerRuleViewTestSuite () {
244+
let cn = "UnitTestsOptimizer";
245+
246+
return {
247+
248+
setUp : function () {
249+
db._dropView(cn + "View");
250+
db._drop(cn);
251+
db._create(cn);
252+
< F438 span class=pl-s1>db._createView(cn + "View", "arangosearch", { links: { "UnitTestsOptimizer" : { includeAllFields: true } } });
253+
},
254+
255+
tearDown : function () {
256+
db._dropView(cn + "View");
257+
db._drop(cn);
258+
},
259+
260+
testVariableReplacementInSearchCondition : function () {
261+
let query = "LET sub = (RETURN 1) FOR outer IN sub FOR v IN " + cn + "View SEARCH v.something == outer RETURN v";
262+
263+
let result = AQL_EXPLAIN(query);
264+
assertNotEqual(-1, result.plan.rules.indexOf(ruleName), query);
265+
266+
let nodes = helper.removeClusterNodesFromPlan(result.plan.nodes);
267+
268+
assertEqual("ReturnNode", nodes[nodes.length - 1].type);
269+
assertEqual("EnumerateViewNode", nodes[nodes.length - 2].type);
270+
271+
let viewNode = nodes[nodes.length - 2];
272+
assertEqual(cn + "View", viewNode.view);
273+
assertEqual("v", viewNode.outVariable.name);
274+
assertEqual("n-ary or", viewNode.condition.type);
275+
assertEqual("n-ary and", viewNode.condition.subNodes[0].type);
276+
assertEqual("compare ==", viewNode.condition.subNodes[0].subNodes[0].type);
277+
assertEqual("attribute access", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].type);
278+
assertEqual("something", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].name);
279+
assertEqual("reference", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].type);
280+
assertEqual("v", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].name);
281+
assertEqual("reference", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].type);
282+
assertEqual("outer", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].name);
283+
assertEqual([], viewNode.sortCondition);
284+
},
285+
286+
testNoVariableReplacementInSearchCondition : function () {
287+
let query = "LET sub = (RETURN 1) FOR outer IN sub FOR v IN " + cn + "View SEARCH v.something == 1 RETURN v";
288+
289+
let result = AQL_EXPLAIN(query);
290+
assertNotEqual(-1, result.plan.rules.indexOf(ruleName), query);
291+
292+
let nodes = helper.removeClusterNodesFromPlan(result.plan.nodes);
293+
294+
assertEqual("ReturnNode", nodes[nodes.length - 1].type);
295+
assertEqual("EnumerateViewNode", nodes[nodes.length - 2].type);
296+
297+
let viewNode = nodes[nodes.length - 2];
298+
assertEqual(cn + "View", viewNode.view);
299+
assertEqual("v", viewNode.outVariable.name);
300+
assertEqual("n-ary or", viewNode.condition.type);
301+
assertEqual("n-ary and", viewNode.condition.subNodes[0].type);
302+
assertEqual("compare ==", viewNode.condition.subNodes[0].subNodes[0].type);
303+
assertEqual("attribute access", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].type);
304+
assertEqual("something", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].name);
305+
assertEqual("reference", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].type);
306+
assertEqual("v", viewNode.condition.subNodes[0].subNodes[0].subNodes[0].subNodes[0].name);
307+
assertEqual("value", viewNode.condition.subNodes[0].subNodes[0].subNodes[1].type);
308+
assertEqual([], viewNode.sortCondition);
309+
},
310+
311+
};
312+
}
250313

251314
jsunity.run(optimizerRuleTestSuite);
252315
jsunity.run(optimizerRuleCollectionTestSuite);
316+
jsunity.run(optimizerRuleViewTestSuite);
253317

254318
return jsunity.done();

0 commit comments

Comments
 (0)
0