8000 fixed issue #16279 · arangodb/arangodb@a162566 · GitHub
[go: up one dir, main page]
Skip to content

Commit a162566

Browse files
jsteemannjsteemann
committed
fixed issue #16279 
1 parent c578ba7 commit a162566

4 files changed

+117
-101
lines changed

‎CHANGELOG

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

4+
* Fixed Github issue #16279: assertion failure/crash in AQL query optimizer
5+
when permuting adjacent FOR loops that depended on each other.
6+
47
* Fixed a potential hang on shutdown, when there were still document operations
58
queued.
69

‎arangod/Aql/OptimizerRules.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3683,6 +3683,8 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(Optimizer* opt,
36833683
std::vector<size_t> starts;
36843684
std::vector<ExecutionNode*> nn;
36853685

3686+
VarSet inputVars;
3687+
36863688
// We use that the order of the nodes is such that a node B that is among the
36873689
// recursive dependencies of a node A is later in the vector.
36883690
for (auto const& n : nodes) {
@@ -3691,6 +3693,8 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(Optimizer* opt,
36913693
nn.emplace_back(n);
36923694
nodesSet.erase(n);
36933695

3696+
inputVars.clear();
3697+
36943698
// Now follow the dependencies as long as we see further such nodes:
36953699
auto nwalker = n;
36963700

@@ -3701,6 +3705,9 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(Optimizer* opt,
37013705

37023706
auto dep = nwalker->getFirstDependency();
37033707

3708+
// track variables that we rely on
3709+
nwalker->getVariablesUsedHere(inputVars);
3710+
37043711
if (dep->getType() != EN::ENUMERATE_COLLECTION && dep->getType() != EN::ENUMERATE_LIST) {
37053712
break;
37063713
}
@@ -3709,6 +3716,26 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(Optimizer* opt,
37093716
break;
37103717
}
37113718

3719+
// check if nodes depend on each other (i.e. node C consumes a variable
3720+
// introduced by node B or A):
3721+
// - FOR a IN A
3722+
// - FOR b IN a.values
3723+
// - FOR c IN b.values
3724+
// or
3725+
// - FOR a IN A
3726+
// - FOR b IN ...
3727+
// - FOR c IN a.values
3728+
bool foundDependency = false;
3729+
for (auto const& outVar : dep->getVariablesSetHere()) {
3730+
if (inputVars.contains(outVar)) {
3731+
foundDependency = true;
3732+
break;
3733+
}
3734+
}
3735+
if (foundDependency) {
3736+
break;
3737+
}
3738+
37123739
nwalker = dep;
37133740
nn.emplace_back(nwalker);
37143741
nodesSet.erase(nwalker);

‎tests/js/server/aql/aql-optimizer-rule-interchange-adjacent-enumerations-cluster.js

Lines changed: 41 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,46 +28,39 @@
2828
/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
2929
////////////////////////////////////////////////////////////////////////////////
3030

31-
var jsunity = require("jsunity");
32-
var helper = require("@arangodb/aql-helper");
33-
var isEqual = helper.isEqual;
34-
var db = require("@arangodb").db;
35-
var _ = require("lodash");
31+
const jsunity = require("jsunity");
32+
const helper = require("@arangodb/aql-helper");
33+
const isEqual = helper.isEqual;
34+
const db = require("@arangodb").db;
35+
const _ = require("lodash");
3636

3737
////////////////////////////////////////////////////////////////////////////////
3838
/// @brief test suite
3939
////////////////////////////////////////////////////////////////////////////////
4040

4141
function optimizerRuleTestSuite () {
42-
var ruleName = "interchange-adjacent-enumerations";
42+
const ruleName = "interchange-adjacent-enumerations";
4343

4444
// various choices to control the optimizer:
45-
var paramNone = { optimizer: { rules: [ "-all" ] } };
46-
var paramEnabled = { optimizer: { rules: [ "-all", "+" + ruleName ] } };
47-
var paramDisabled = { optimizer: { rules: [ "+all", "-" + ruleName ] } };
45+
const paramNone = { optimizer: { rules: [ "-all" ] } };
46+
const paramEnabled = { optimizer: { rules: [ "-all", "+" + ruleName ] } };
47+
const paramDisabled = { optimizer: { rules: [ "+all", "-" + ruleName ] } };
4848

49-
var collection = null;
50-
var collectionName = "UnitTestsAhuacatlOptimizer";
49+
const collectionName = "UnitTestsAhuacatlOptimizer";
5150

5251
return {
5352

54-
////////////////////////////////////////////////////////////////////////////////
55-
/// @brief set up
56-
////////////////////////////////////////////////////////////////////////////////
57-
5853
setUpAll : function () {
5954
db._drop(collectionName);
60-
collection = db._create(collectionName);
55+
let collection = db._create(collectionName);
6156

62-
for (var i = 0; i < 10; ++i) {
63-
collection.save({ value: i });
57+
let docs = [];
58+
for (let i = 0; i < 10; ++i) {
59+
docs.push({ value: i });
6460
}
61+
collection.insert(docs);
6562
},
6663

67-
////////////////////////////////////////////////////////////////////////////////
68-
/// @brief tear down
69-
////////////////////////////////////////////////////////////////////////////////
70-
7164
tearDownAll : function () {
7265
db._drop(collectionName);
7366
},
@@ -77,17 +70,17 @@ function optimizerRuleTestSuite () {
7770////////////////////////////////////////////////////////////////////////////////
7871

7972
testRuleDisabled : function () {
80-
var queries = [
73+
let queries = [
8174
"FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN 1",
8275
"FOR j IN " + collectionName + " FILTER j.i == 1 FOR i IN " + collectionName + " RETURN j"
8376
];
8477

85-
var opts = _.clone(paramNone);
78+
let opts = _.clone(paramNone);
8679
opts.allPlans = true;
8780
opts.verbosePlans = true;
8881

8982
queries.forEach(function(query) {
90-
var result = AQL_EXPLAIN(query, { }, opts);
83+
let result = AQL_EXPLAIN(query, { }, opts);
9184
result.plans.forEach(function(plan) {
9285
assertEqual([ "scatter-in-cluster" ], plan.rules);
9386
});
@@ -99,22 +92,27 @@ function optimizerRuleTestSuite () {
9992
////////////////////////////////////////////////////////////////////////////////
10093

10194
testRuleNoEffect : function () {
102-
var queries = [
95+
let queries = [
10396
"FOR i IN 1..10 RETURN i",
10497
"FOR i IN " + collectionName + " RETURN i",
10598
"FOR i IN " + collectionName + " FILTER i == 1 FOR j IN " + collectionName + " RETURN i",
10699
"FOR i IN " + collectionName + " LIMIT 1 FOR j IN " + collectionName + " RETURN i",
107-
"FOR i IN " + collectionName + " RETURN (FOR j IN " + collectionName + " RETURN j)"
100+
"FOR i IN " + collectionName + " RETURN (FOR j IN " + collectionName + " RETURN j)",
101+
// the following query must not be optimized because "sub" depends on "i"
102+
"FOR i IN " + collectionName + " FOR sub IN i FILTER sub.value1 == 'test' && sub.value2 != '' RETURN i",
103+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN sub1 FILTER sub2.value1 == 'test' && sub2.value2 != '' RETURN i",
104+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN i FILTER sub2.value1 == 'test' && sub2.value2 != '' RETURN i",
105+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN i FILTER sub2.value1 == 'test' && sub2.value2 != '' && sub2.value != sub1 RETURN i",
108106
];
109107

110-
var opts = _.clone(paramEnabled);
108+
let opts = _.clone(paramEnabled);
111109
opts.allPlans = true;
112110
opts.verbosePlans = true;
113111

114112
queries.forEach(function(query) {
115-
var result = AQL_EXPLAIN(query, { }, opts);
113+
let result = AQL_EXPLAIN(query, { }, opts);
116114
result.plans.forEach(function(plan) {
117-
assertTrue(plan.rules.indexOf(ruleName) === -1, query);
115+
assertEqual(-1, plan.rules.indexOf(ruleName), query);
118116
});
119117
});
120118
},
@@ -124,7 +122,7 @@ function optimizerRuleTestSuite () {
124122
////////////////////////////////////////////////////////////////////////////////
125123

126124
testRuleHasEffect : function () {
127-
var queries = [
125+
let queries = [
128126
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN i", 1 ],
129127
[ "FOR i IN 1..10 FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i", 5 ],
130128
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i", 5 ],
@@ -134,15 +132,15 @@ function optimizerRuleTestSuite () {
134132
[ "FOR x IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i) FOR y IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN i) RETURN x", 11 ]
135133
];
136134

137-
var opts = _.clone(paramEnabled);
135+
let opts = _.clone(paramEnabled);
138136
opts.allPlans = true;
139137
opts.verbosePlans = true;
140138

141139
queries.forEach(function(query) {
142-
var withRule = 0;
143-
var withoutRule = 0;
140+
let withRule = 0;
141+
let withoutRule = 0;
144142

145-
var result = AQL_EXPLAIN(query[0], { }, opts);
143+
let result = AQL_EXPLAIN(query[0], { }, opts);
146144
result.plans.forEach(function(plan) {
147145
if (plan.rules.indexOf(ruleName) === -1) {
148146
withoutRule++;
@@ -165,30 +163,30 @@ function optimizerRuleTestSuite () {
165163
////////////////////////////////////////////////////////////////////////////////
166164

167165
testResults : function () {
168-
var queries = [
166+
let queries = [
169167
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " SORT i.value, j.value FILTER i.value == j.value RETURN i.value", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ],
170168
[ "FOR j IN " + collectionName + " FOR i IN " + collectionName + " SORT i.value, j.value FILTER i.value == j.value RETURN i.value", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ],
171169
[ "FOR x IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN { i: i.value, j: j.value }) FILTER x.i == x.j SORT x.i RETURN x.i", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ]
172170
];
173171

174-
var opts = _.clone(paramEnabled);
172+
let opts = _.clone(paramEnabled);
175173
opts.allPlans = true;
176174
opts.verbosePlans = true;
177175

178176
queries.forEach(function(query) {
179-
var planDisabled = AQL_EXPLAIN(query[0], { }, paramDisabled);
180-
var plansEnabled = AQL_EXPLAIN(query[0], { }, opts);
181-
var resultDisabled = AQL_EXECUTE(query[0], { }, paramDisabled).json;
177+
let planDisabled = AQL_EXPLAIN(query[0], { }, paramDisabled);
178+
let plansEnabled = AQL_EXPLAIN(query[0], { }, opts);
179+
let resultDisabled = AQL_EXECUTE(query[0], { }, paramDisabled).json;
182180

183-
assertTrue(planDisabled.plan.rules.indexOf(ruleName) === -1, query[0]);
181+
assertEqual(-1, planDisabled.plan.rules.indexOf(ruleName), query[0]);
184182
assertEqual(resultDisabled, query[1]);
185183

186184
assertTrue(plansEnabled.plans.length > 1);
187185

188186
// iterate over all plans
189-
var withRule = 0;
187+
let withRule = 0;
190188
plansEnabled.plans.forEach(function(plan) {
191-
var resultEnabled = AQL_EXECUTEJSON(plan).json;
189+
let resultEnabled = AQL_EXECUTEJSON(plan).json;
192190
assertTrue(isEqual(resultDisabled, resultEnabled), query[0]);
193191
if (plan.rules.indexOf(ruleName) !== -1) {
194192
withRule++;
@@ -204,11 +202,6 @@ function optimizerRuleTestSuite () {
204202
};
205203
}
206204

207-
////////////////////////////////////////////////////////////////////////////////
208-
/// @brief executes the test suite
209-
////////////////////////////////////////////////////////////////////////////////
210-
211205
jsunity.run(optimizerRuleTestSuite);
212206

213207
return jsunity.done();
214-

0 commit comments

Comments
 (0)
0