8000 [3.5] Fixed AR-113 and added regression tests by goedderz · Pull Request #13241 · arangodb/arangodb · GitHub
[go: up one dir, main page]
Skip to content

[3.5] Fixed AR-113 and added regression tests #13241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 21, 2020
Merged

[3.5] Fixed AR-113 and added regression tests #13241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
8000
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
v3.5.7 (XXXX-XX-XX)
-------------------

* Fix AR-113. Disallow non-values in the AQL geo-index-optimizer rule.

* Updated ArangoDB Starter to 0.14.15-1.

* Updated arangosync to 0.7.13.
Expand Down
8 changes: 2 additions & 6 deletions arangod/Aql/OptimizerRules.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6213,10 +6213,6 @@ void arangodb::aql::inlineSubqueriesRule(Optimizer* opt, std::unique_ptr<Executi
opt->addPlan(std::move(plan), rule, modified);
}

static bool isValueOrReference(AstNode const* node) {
return node->type == NODE_TYPE_VALUE || node->type == NODE_TYPE_REFERENCE;
}

/// Essentially mirrors the geo::QueryParams struct, but with
/// abstracts AstNode value objects
struct GeoIndexInfo {
Expand Down Expand Up @@ -6522,15 +6518,15 @@ bool checkGeoFilterExpression(ExecutionPlan* plan, AstNode const* node, GeoIndex
// checks @first `smaller` @second
// note: this only modifies "info" if the function returns true
auto eval = [&](AstNode const* first, AstNode const* second, bool lessequal) -> bool {
if (isValueOrReference(second) && // no attribute access
if (second->type == NODE_TYPE_VALUE && // only constants allowed
info.maxDistanceExpr == nullptr && // max distance is not yet set
checkDistanceFunc(plan, first, /*legacy*/ true, info)) {
TRI_ASSERT(info.index);
info.maxDistanceExpr = second;
info.maxInclusive = info.maxInclusive && lessequal;
info.nodesToRemove.insert(node);
return true;
} else if (isValueOrReference(first) && // no attribute access
} else if (first->type == NODE_TYPE_VALUE && // only constants allowed
info.minDistanceExpr == nullptr && // min distance is not yet set
checkDistanceFunc(plan, second, /*legacy*/ true, info)) {
info.minDistanceExpr = first;
Expand Down
11 changes: 9 additions & 2 deletions arangod/GeoIndex/Index.cpp
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
case aql::NODE_TYPE_OPERATOR_BINARY_LE:
qp.maxInclusive = true;
// intentional fallthrough
[[fallthrough]];
case aql::NODE_TYPE_OPERATOR_BINARY_LT: {
TRI_ASSERT(node->numMembers() == 2);
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
Expand All @@ -255,6 +256,9 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,

aql::AstNode const* max = node->getMemberUnchecked(1);
TRI_ASSERT(max->type == aql::NODE_TYPE_VALUE);
if (max->type != aql::NODE_TYPE_VALUE) {
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
}
if (!max->isValueType(aql::VALUE_TYPE_STRING)) {
qp.maxDistance = max->getDoubleValue();
} // else assert(max->getStringValue() == "unlimited")
Expand All @@ -263,22 +267,25 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
case aql::NODE_TYPE_OPERATOR_BINARY_GE:
qp.minInclusive = true;
// intentional fallthrough
[[fallthrough]];
case aql::NODE_TYPE_OPERATOR_BINARY_GT: {
TRI_ASSERT(node->numMembers() == 2);
qp.origin = Index::parseDistFCall(node->getMemberUnchecked(0), ref);
if (!qp.origin.is_valid()) {
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
}
// LOG_TOPIC("a9633", ERR, Logger::FIXME) << "Found center: " << c.toString();

aql::AstNode const* min = node->getMemberUnchecked(1);
TRI_ASSERT(min->type == aql::NODE_TYPE_VALUE);
if (min->type != aql::NODE_TYPE_VALUE) {
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
}
qp.minDistance = min->getDoubleValue();
break;
}
default:
TRI_ASSERT(false);
break;
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
}
}

Expand Down
81 changes: 80 additions & 1 deletion tests/js/server/aql/aql-optimizer-geoindex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,86 @@ function optimizerRuleTestSuite() {
var result = AQL_EXPLAIN(query.string, query.bindVars);
hasIndexNode(result, query);
hasNoFilterNode(result, query);
}
},

// Regression test (https://arangodb.atlassian.net/browse/AR-113):
// Previously, the filter was moved into the index, but one access of `dist` in `dist < dist` was left, while the
// variable obviously can't be available yet at the index node.
// When failing, throws an error like
// missing variable #1 (dist) for node #8 (IndexNode) while planning registers
testSelfReference1: function () {
const query = {
stri 8000 ng: `
FOR doc IN @@col
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
FILTER dist < dist
RETURN {doc, dist}
`,
bindVars: {'@col': colName},
};

const result = AQL_EXPLAIN(query.string, query.bindVars);
hasNoIndexNode(result, query);
hasFilterNode(result, query);
},

// See testSelfReference1
testSelfReference2: function () {
const query = {
string: `
FOR doc IN @@col
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
LET dist2 = GEO_DISTANCE(doc.geo, [3, 7])
FILTER dist < dist2
RETURN {doc, dist, dist2}
`,
bindVars: {'@col': colName},
};

const result = AQL_EXPLAIN(query.string, query.bindVars);
hasNoIndexNode(result, query);
hasFilterNode(result, query);
},

// See testSelfReference1
testSelfReference3: function () {
const query = {
string: `
FOR doc IN @@col
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
LET dist2 = doc.maxDist
FILTER dist < dist2
RETURN {doc, dist, dist2}
`,
bindVars: {'@col': colName},
};

const result = AQL_EXPLAIN(query.string, query.bindVars);
hasNoIndexNode(result, query);
hasFilterNode(result, query);
},

// See testSelfReference1
// Note that currently, the optimizer rule does not work with `dist2` being
// not a value (but a variable reference) in the filter expression. When
// this changes in the future, the expectations in this test can simply be
// changed.
testInaccessibleReference: function () {
const query = {
string: `
FOR doc IN @@col
LET dist = GEO_DISTANCE(doc.geo, [16, 53])
LET dist2 = NOEVAL(7)
FILTER dist < dist2
RETURN {doc, dist, dist2}
`,
bindVars: {'@col': colName},
};

const result = AQL_EXPLAIN(query.string, query.bindVars);
hasNoIndexNode(result, query);
hasFilterNode(result, query);
},

}; // test dictionary (return)
} // optimizerRuleTestSuite
Expand Down
0