8000 Fixed AR-113 and added regression tests (#13196) · arangodb/arangodb@264d298 · GitHub
[go: up one dir, main page]
Skip to content

Commit 264d298

Browse files
goedderzjsteemann
goedderz
and
jsteemann
committed
Fixed AR-113 and added regression tests (#13196)
Co-authored-by: jsteemann <jan@arangodb.com>
1 parent 7b67883 commit 264d298

File tree

4 files changed

+93
-9
lines changed

4 files changed

+93
-9
lines changed

‎CHANGELOG

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

4+
* Fix AR-113. Disallow non-values in the AQL geo-index-optimizer rule.
5+
46
* Updated ArangoDB Starter to 0.14.15-1.
57

68
* Updated arangosync to 0.7.13.

‎arangod/Aql/OptimizerRules.cpp

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6213,10 +6213,6 @@ void arangodb::aql::inlineSubqueriesRule(Optimizer* opt, std::unique_ptr<Executi
62136213
opt->addPlan(std::move(plan), rule, modified);
62146214
}
62156215

6216-
static bool isValueOrReference(AstNode const* node) {
6217-
return node->type == NODE_TYPE_VALUE || node->type == NODE_TYPE_REFERENCE;
6218-
}
6219-
62206216
/// Essentially mirrors the geo::QueryParams struct, but with
62216217
/// abstracts AstNode value objects
62226218
struct GeoIndexInfo {
@@ -6522,15 +6518,15 @@ bool checkGeoFilterExpression(ExecutionPlan* plan, AstNode const* node, GeoIndex
65226518
// checks @first `smaller` @second
65236519
// note: this only modifies "info" if the function returns true
65246520
auto eval = [&](AstNode const* first, AstNode const* second, bool lessequal) -> bool {
6525-
if (isValueOrReference(second) && // no attribute access
6521+
if (second->type == NODE_TYPE_VALUE && // only constants allowed
65266522
info.maxDistanceExpr == nullptr && // max distance is not yet set
65276523
checkDistanceFunc(plan, first, /*legacy*/ true, info)) {
65286524
TRI_ASSERT(info.index);
65296525
info.maxDistanceExpr = second;
65306526
info.maxInclusive = info.maxInclusive && lessequal;
65316527
info.nodesToRemove.insert(node);
65326528
return true;
6533-
} else if (isValueOrReference(first) && // no attribute access
6529+
} else if (first->type == NODE_TYPE_VALUE && // only constants allowed
65346530
info.minDistanceExpr == nullptr && // min distance is not yet set
65356531
checkDistanceFunc(plan, second, /*legacy*/ true, info)) {
65366532
info.minDistanceExpr = first;

‎arangod/GeoIndex/Index.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
246246
case aql::NODE_TYPE_OPERATOR_BINARY_LE:
247247
qp.maxInclusive = true;
248248
// intentional fallthrough
249+
[[fallthrough]];
249250
case aql::NODE_TYPE_OPERATOR_BINARY_LT: {
250251
TRI_ASSERT(node->numMembers() == 2);
251252
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
@@ -255,6 +256,9 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
255256

256257
aql::AstNode const* max = node->getMemberUnchecked(1);
257258
TRI_ASSERT(max->type == aql::NODE_TYPE_VALUE);
259+
if (max->type != aql::NODE_TYPE_VALUE) {
260+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
261+
}
258262
if (!max->isValueType(aql::VALUE_TYPE_STRING)) {
259263
qp.maxDistance = max->getDoubleValue();
260264
} // else assert(max->getStringValue() == "unlimited")
@@ -263,22 +267,25 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
263267
case aql::NODE_TYPE_OPERATOR_BINARY_GE:
264268
qp.minInclusive = true;
265269
// intentional fallthrough
270+
[[fallthrough]];
266271
case aql::NODE_TYPE_OPERATOR_BINARY_GT: {
267272
TRI_ASSERT(node->numMembers() == 2);
268273
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
269274
if (!qp.origin.is_valid()) {
270275
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
271276
}
272-
// LOG_TOPIC("a9633", ERR, Logger::FIXME) << "Found center: " << c.toString();
273277

274278
aql::AstNode const* min = node->getMemberUnchecked(1);
275279
TRI_ASSERT(min->type == aql::NODE_TYPE_VALUE);
280+
if (min->type != aql::NODE_TYPE_VALUE) {
281+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
282+
}
276283
qp.minDistance = min->getDoubleValue();
277284
break;
278285
}
279286
default:
280287
TRI_ASSERT(false);
281-
break;
288+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
282289
}
283290
}
284291

‎tests/js/server/aql/aql-optimizer-geoindex.js

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1039,7 +1039,86 @@ function optimizerRuleTestSuite() {
10391039
var result = AQL_EXPLAIN(query.string, query.bindVars);
10401040
hasIndexNode(result, query);
10411041
hasNoFilterNode(result, query);
1042-
}
1042+
},
1043+
1044+
// Regression test (https://arangodb.atlassian.net/browse/AR-113):
1045+
// Previously, the filter was moved into the index, but one access of `dist` in `dist < dist` was left, while the
1046+
// variable obviously can't be available yet at the index node.
1047+
// When failing, throws an error like
1048+
// missing variable #1 (dist) for node #8 (IndexNode) while planning registers
1049+
testSelfReference1: function () {
1050+
const query = {
1051+
string: `
1052+
FOR doc IN @@col
1053+
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
1054+
FILTER dist < dist
1055+
RETURN {doc, dist}
1056+
`,
1057+
bindVars: {'@col': colName},
1058+
};
1059+
1060+
const result = AQL_EXPLAIN(query.string, query.bindVars);
1061+
hasNoIndexNode(result, query);
1062+
hasFilterNode(result, query);
1063+
},
1064+
1065+
// See testSelfReference1
1066+
testSelfReference2: function () {
1067+
const query = {
1068+
string: `
1069+
FOR doc IN @@col
1070+
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
1071+
LET dist2 = GEO_DISTANCE(doc.geo, [3, 7])
1072+
FILTER dist < dist2
1073+
RETURN {doc, dist, dist2}
1074+
`,
1075+
bindVars: {'@col': colName},
1076+
};
1077+
1078+
const result = AQL_EXPLAIN(query.string, query.bindVars);
1079+
hasNoIndexNode(result, query);
1080+
hasFilterNode(result, query);
1081+
},
1082+
1083+
// See testSelfReference1
1084+
testSelfReference3: function () {
1085+
const query = {
1086+
string: `
1087+
FOR doc IN @@col
1088+
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
1089+
LET dist2 = doc.maxDist
1090+
FILTER dist < dist2
1091+
RETURN {doc, dist, dist2}
1092+
`,
1093+
bindVars: {'@col': colName},
1094+
};
1095+
1096+
const result = AQL_EXPLAIN(query.string, query.bindVars);
1097+
hasNoIndexNode(result, query);
1098+
hasFilterNode(result, query);
1099+
},
1100+
1101+
// See testSelfReference1
1102+
// Note that currently, the optimizer rule does not work with `dist2` being
1103+
// not a value (but a variable reference) in the filter expression. When
1104+
// this changes in the future, the expectations in this test can simply be
1105+
// changed.
1106+
testInaccessibleReference: function () {
1107+
const query = {
1108+
string: `
1109+
FOR doc IN @@col
1110+
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
1111+
LET dist2 = NOEVAL(7)
1112+
FILTER dist < dist2
1113+
RETURN {doc, dist, dist2}
1114+
`,
1115+
bindVars: {'@col': colName},
1116+
};
1117+
1118+
const result = AQL_EXPLAIN(query.string, query.bindVars);
1119+
hasNoIndexNode(result, query);
1120+
hasFilterNode(result, query);
1121+
},
10431122

10441123
}; // test dictionary (return)
10451124
} // optimizerRuleTestSuite

0 commit comments

Comments
 (0)
0