8000 [3.3] Fixed optimize-traversals and added a regression test (#6976) · strogo/arangodb@ed41d8e · GitHub
[go: up one dir, main page]
Skip to content

Commit ed41d8e

Browse files
goedderzmchacki
goedderz
authored and
mchacki
committed
[3.3] Fixed optimize-traversals and added a regression test (arangodb#6976)
* Fixed optimize-traversals and added a regression test * Updated CHANGELOG
1 parent b23ba07 commit ed41d8e

File tree

3 files changed

+81
-19
lines changed

3 files changed

+81
-19
lines changed

‎CHANGELOG

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
v3.3.20 (XXXX-XX-XX)
22
--------------------
33

4+
* Fixed an AQL bug where the optimize-traversals rule was falsely applied to
5+
extensions with inline expressions and thereby ignoring them
6+
47
* fix side-effects of sorting larger arrays (>= 16 members) of constant literal
58
values in AQL, when the array was not used only for IN-value filtering but also
69
later in the query.

‎arangod/Aql/TraversalConditionFinder.cpp

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ static bool checkPathVariableAccessFeasible(Ast* ast, AstNode* parent,
235235
// A) var.vertices[n] (.*)
236236
// B) var.edges[n] (.*)
237237
// C) var.vertices[*] (.*) (ALL|NONE) (.*)
238-
// D) var.vertices[*] (.*) (ALL|NONE) (.*)
238+
// D) var.edges[*] (.*) (ALL|NONE) (.*)
239239

240240
auto unusedWalker = [](AstNode const* n, void*) {};
241241
bool isEdge = false;
@@ -340,6 +340,18 @@ static bool checkPathVariableAccessFeasible(Ast* ast, AstNode* parent,
340340
return node;
341341
}
342342
if (node->type == NODE_TYPE_EXPANSION) {
343+
344+
// Check that the expansion [*] contains no inline expression;
345+
// members 2, 3 and 4 correspond to FILTER, LIMIT and RETURN,
346+
// respectively.
347+
TRI_ASSERT(node->numMembers() == 5);
348+
if (node->getMemberUnchecked(2)->type != NODE_TYPE_NOP
349+
|| node->getMemberUnchecked(3)->type != NODE_TYPE_NOP
350+
|| node->getMemberUnchecked(4)->type != NODE_TYPE_NOP) {
351+
notSupported = true;
352+
return node;
353+
}
354+
343355
// We continue in this pattern, all good
344356
patternStep++;
345357
parentOfReplace = node;

‎js/server/tests/aql/aql-optimizer-rule-optimize-traversals-spec.js

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,36 @@ const cleanup = () => {
5050
} catch (x) {
5151
}
5252
};
53+
54+
const assertRuleIsUsed = (query) => {
55+
// assert the rule is not used when it's disabled
56+
const planDisabled = AQL_EXPLAIN(query, { }, paramDisabled);
57+
expect(planDisabled.plan.rules.indexOf(ruleName)).to.equal(-1, query);
58+
59+
// assert the rule is used when it's enabled
60+
const planEnabled = AQL_EXPLAIN(query, { }, paramEnabled);
61+
expect(planEnabled.plan.rules.indexOf(ruleName)).to.not.equal(-1, query);
62+
};
63+
64+
const assertResultsAreUnchanged = (query) => {
65+
// assert the rule is not used when it's disabled.
66+
const planDisabled = AQL_EXPLAIN(query, { }, paramDisabled);
67+
expect(planDisabled.plan.rules.indexOf(ruleName)).to.equal(-1, query);
68+
69+
const resultDisabled = AQL_EXECUTE(query, { }, paramDisabled).json;
70+
const resultEnabled = AQL_EXECUTE(query, { }, paramEnabled).json;
71+
72+
expect(isEqual(resultDisabled, resultEnabled)).to.equal(true, query);
73+
74+
const opts = { allPlans: true, verbosePlans: true, optimizer: { rules: [ '-all', '+' + ruleName ] } };
75+
76+
const plans = AQL_EXPLAIN(query, {}, opts).plans;
77+
plans.forEach(function (plan) {
78+
const jsonResult = AQL_EXECUTEJSON(plan, { optimizer: { rules: [ '-all' ] } }).json;
79+
expect(jsonResult).to.deep.equal(resultDisabled, query);
80+
});
81+
};
82+
5383
// //////////////////////////////////////////////////////////////////////////////
5484
// / @brief test suite
5585
// //////////////////////////////////////////////////////////////////////////////
@@ -207,25 +237,9 @@ describe('Rule optimize-traversals', () => {
207237
FILTER p.edges[1].theTruth == true FILTER p.edges[1].label == 'foo'
208238
RETURN {v,e,p}`
209239
];
210-
const opts = { allPlans: true, verbosePlans: true, optimizer: { rules: [ '-all', '+' + ruleName ] } };
211-
212-
queries.forEach(function (query) {
213-
const planDisabled = AQL_EXPLAIN(query, { }, paramDisabled);
214-
const planEnabled = AQL_EXPLAIN(query, { }, paramEnabled);
215-
const resultDisabled = AQL_EXECUTE(query, { }, paramDisabled).json;
216-
const resultEnabled = AQL_EXECUTE(query, { }, paramEnabled).json;
217240

218-
expect(isEqual(resultDisabled, resultEnabled)).to.equal(true, query);
219-
220-
expect(planDisabled.plan.rules.indexOf(ruleName)).to.equal(-1, query);
221-
expect(planEnabled.plan.rules.indexOf(ruleName)).to.not.equal(-1, query);
222-
223-
const plans = AQL_EXPLAIN(query, {}, opts).plans;
224-
plans.forEach(function (plan) {
225-
const jsonResult = AQL_EXECUTEJSON(plan, { optimizer: { rules: [ '-all' ] } }).json;
226-
expect(jsonResult).to.deep.equal(resultDisabled, query);
227-
});
228-
});
241+
queries.forEach(query => assertRuleIsUsed(query));
242+
queries.forEach(query => assertResultsAreUnchanged(query));
229243
});
230244

231245
it('should prune when using functions', () => {
@@ -655,4 +669,37 @@ describe('Rule optimize-traversals', () => {
655669
});
656670
});
657671
});
672+
673+
describe('regression tests', () => {
674+
before(cleanup);
675+
after(cleanup);
676+
677+
// https://github.com/arangodb/release-3.4/issues/91
678+
// Regression test: With 'optimize-traversals' enabled, the filter
679+
// p.vertices[* FILTER CURRENT._id != 'V/1'].label ALL == true
680+
// was effectively changed to
681+
// p.vertices[*].label ALL == true
682+
// . That is, inline expressions were ignored.
683+
it('inline expressions should not be changed', () => {
684+
{ // create data
685+
graph = graphModule._create(graphName, [
686+
graphModule._relation('E', 'V', 'V')]);
687+
688+
graph.V.save({_key: '1', label: false});
689+
graph.V.save({_key: '2', label: true});
690+
graph.V.save({_key: '3', label: false});
691+
692+
graph.E.save('V/1', 'V/2', {});
693+
graph.E.save('V/1', 'V/3', {});
694+
}
695+
696+
const query = `
697+
FOR v, e, p IN 1..10 OUTBOUND 'V/1' GRAPH '${graphName}'
698+
FILTER p.vertices[* FILTER CURRENT._id != 'V/1'].label ALL == true
699+
RETURN p.vertices
700+
`;
701+
702+
assertResultsAreUnchanged(query);
703+
});
704+
});
658705
});

0 commit comments

Comments
 (0)
0