arangodb · jsteemann · Dec 15, 2020 · Dec 10, 2020 · Dec 11, 2020 · Dec 14, 2020
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,9 +1,14 @@
 devel
 -----
+
 * Fix for issue #772: Optimized document counting for ArangoSearch views.
   Added new ArangoSearch view option 'countApproximate' for customizing 
   view count strategy.
 
+* Fix AR-113. Add a check in the AQL geo-index-optimizer rule that vars
+  referenced by a candidate index expression are actually available at the
+  candidate index node.
+
 * Views on SmartGraph Edge collections do not contain some documents
   twice.
 

diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp
@@ -6507,10 +6507,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 {
@@ -6814,15 +6810,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;

diff --git a/arangod/Cluster/RestClusterHandler.cpp b/arangod/Cluster/RestClusterHandler.cpp
@@ -154,7 +154,6 @@ void RestClusterHandler::handleClusterInfo() {
   auto dump = ci.toVelocyPack();
 
   generateResult(rest::ResponseCode::OK, dump.slice());
-
 }
 
 /// @brief returns information about all coordinator endpoints

diff --git a/arangod/GeoIndex/Index.cpp b/arangod/GeoIndex/Index.cpp
@@ -251,6 +251,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);
@@ -260,6 +261,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")
@@ -268,22 +272,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);
   }
 }
 

diff --git a/tests/js/server/aql/aql-optimizer-geoindex.js b/tests/js/server/aql/aql-optimizer-geoindex.js
@@ -1029,7 +1029,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 = {
+        string: `
+          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