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

Commit f779e91

Browse files
goedderzjsteemannKVS85
authored
Fixed AR-113 and added regression tests (#13196) (#13240)
Co-authored-by: jsteemann <jan@arangodb.com> Co-authored-by: jsteemann <jan@arangodb.com> Co-authored-by: Vadim <vadim@arangodb.com>
1 parent 2fce82c commit f779e91

File tree

5 files changed

+93
-10
lines changed

5 files changed

+93
-10
lines changed

‎CHANGELOG

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

4+
* Fix AR-113. Disallow non-values in the AQL geo-index-optimizer rule.
5+
46
* Added SNI support for arangosh.
57

68
* Fix agency restart with mismatching compation and log indexes

‎arangod/Aql/OptimizerRules.cpp

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

6478-
static bool isValueOrReference(AstNode const* node) {
6479-
return node->type == NODE_TYPE_VALUE || node->type == NODE_TYPE_REFERENCE;
6480-
}
6481-
64826478
/// Essentially mirrors the geo::QueryParams struct, but with
64836479
/// abstracts AstNode value objects
64846480
struct GeoIndexInfo {
@@ -6782,15 +6778,15 @@ bool checkGeoFilterExpression(ExecutionPlan* plan, AstNode const* node, GeoIndex
67826778
// checks @first `smaller` @second
67836779
// note: this only modifies "info" if the function returns true
67846780
auto eval = [&](AstNode const* first, AstNode const* second, bool lessequal) -> bool {
6785-
if (isValueOrReference(second) && // no attribute access
6781+
if (second->type == NODE_TYPE_VALUE && // only constants allowed
67866782
info.maxDistanceExpr == nullptr && // max distance is not yet set
67876783
checkDistanceFunc(plan, first, /*legacy*/ true, info)) {
67886784
TRI_ASSERT(info.index);
67896785
info.maxDistanceExpr = second;
67906786
info.maxInclusive = info.maxInclusive && lessequal;
67916787
info.nodesToRemove.insert(node);
67926788
return true;
6793-
} else if (isValueOrReference(first) && // no attribute access
6789+
} else if (first->type == NODE_TYPE_VALUE && // only constants allowed
67946790
info.minDistanceExpr == nullptr && // min distance is not yet set
67956791
checkDistanceFunc(plan, second, /*legacy*/ true, info)) {
67966792
info.minDistanceExpr = first;

‎arangod/Cluster/RestClusterHandler.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,6 @@ void RestClusterHandler::handleClusterInfo() {
153153
auto dump = ci.toVelocyPack();
154154

155155
generateResult(rest::ResponseCode::OK, dump.slice());
156-
157156
}
158157

159158
/// @brief returns information about all coordinator endpoints

‎arangod/GeoIndex/Index.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
250250
case aql::NODE_TYPE_OPERATOR_BINARY_LE:
251251
qp.maxInclusive = true;
252252
// intentional fallthrough
253+
[[fallthrough]];
253254
case aql::NODE_TYPE_OPERATOR_BINARY_LT: {
254255
TRI_ASSERT(node->numMembers() == 2);
255256
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
@@ -259,6 +260,9 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
259260

260261
aql::AstNode const* max = node->getMemberUnchecked(1);
261262
TRI_ASSERT(max->type == aql::NODE_TYPE_VALUE);
263+
if (max->type != aql::NODE_TYPE_VALUE) {
264+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
265+
}
262266
if (!max->isValueType(aql::VALUE_TYPE_STRING)) {
263267
qp.maxDistance = max->getDoubleValue();
264268
} // else assert(max->getStringValue() == "unlimited")
@@ -267,22 +271,25 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
267271
case aql::NODE_TYPE_OPERATOR_BINARY_GE:
268272
qp.minInclusive = true;
269273
// intentional fallthrough
274+
[[fallthrough]];
270275
case aql::NODE_TYPE_OPERATOR_BINARY_GT: {
271276
TRI_ASSERT(node->numMembers() == 2);
272277
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
273278
if (!qp.origin.is_valid()) {
274279
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
275280
}
276-
// LOG_TOPIC("a9633", ERR, Logger::FIXME) << &quo 6D40 t;Found center: " << c.toString();
277281

278282
aql::AstNode const* min = node->getMemberUnchecked(1);
279283
TRI_ASSERT(min->type == aql::NODE_TYPE_VALUE);
284+
if (min->type != aql::NODE_TYPE_VALUE) {
285+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
286+
}
280287
qp.minDistance = min->getDoubleValue();
281288
break;
282289
}
283290
default:
284291
TRI_ASSERT(false);
285-
break;
292+
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
286293
}
287294
}
288295

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

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

10341113
}; // test dictionary (return)
10351114
} // optimizerRuleTestSuite

0 commit comments

Comments
 (0)
0