Fixed BTS-408 (#14513)

CHANGELOG

@@ -1,6 +1,15 @@
 devel
 -----
 
+* Fixed BTS-408: treat positive or negative signed numbers as constants 
+  immediately during AQL query parsing. 
+  Previously, a value of `-1` was parsed initially as `unary minus(value(1))`,
+  which was not treated in the same way as a constant value `value(-1)`. 
+  The value was later optimized to just `value(-1)`, but this only happened 
+  during constant-folding after parsing. Any operations that referred to 
+  the unfolded values during parsing thus did not treat such values as
+  constants. 
+
 * Slightly increase internal AQL query and transaction timeout on DB servers 
   from 3 to 5 minutes.
   Previously, queries and transactions on DB servers could expire quicker,

arangod/Aql/Ast.h

@@ -492,6 +492,11 @@ class Ast {
   /// isValid is set to false, then the returned value is not to be trued and the
   /// the result is equivalent to an AQL `null` value
   static AstNode const* resolveConstAttributeAccess(AstNode const*, bool& isValid);
+
+  /// @brief optimizes the unary operators + and -
+  /// the unary plus will be converted into a simple value node if the operand
+  /// of the operation is a constant number
+  AstNode* optimizeUnaryOperatorArithmetic(AstNode*);
 
  private:
   /// @brief make condition from example
@@ -503,11 +508,6 @@ class Ast {
   /// @brief create a number node for an arithmetic result, double
   AstNode* createArithmeticResultNode(double);
 
-  /// @brief optimizes the unary operators + and -
-  /// the unary plus will be converted into a simple value node if the operand
-  /// of the operation is a constant number
-  AstNode* optimizeUnaryOperatorArithmetic(AstNode*);
-
   /// @brief optimizes the unary operator NOT with a non-constant expression
   AstNode* optimizeNotExpression(AstNode*);
 

arangod/Aql/grammar.cpp

@@ -3951,15 +3951,15 @@ YYLTYPE yylloc = yyloc_default;
   case 127: /* operator_unary: "+ operator" expression  */
 #line 1435 "Aql/grammar.y"
                                   {
-      (yyval.node) = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, (yyvsp[0].node));
+      (yyval.node) = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, (yyvsp[0].node)));
     }
 #line 3957 "Aql/grammar.cpp"
     break;
 
   case 128: /* operator_unary: "- operator" expression  */
 #line 1438 "Aql/grammar.y"
                                     {
-      (yyval.node) = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, (yyvsp[0].node));
+      (yyval.node) = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, (yyvsp[0].node)));
     }
 #line 3965 "Aql/grammar.cpp"
     break;

arangod/Aql/grammar.y

@@ -1433,10 +1433,10 @@ function_call:
 
 operator_unary:
     T_PLUS expression %prec UPLUS {
-      $$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, $2);
+      $$ = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, $2));
     }
   | T_MINUS expression %prec UMINUS {
-      $$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, $2);
+      $$ = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, $2));
     }
   | T_NOT expression %prec UNEGATION {
       $$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_NOT, $2);

arangod/IResearch/IResearchFilterFactory.cpp

@@ -891,44 +891,6 @@ Result byRange(irs::boolean_filter* filter,
   return byRange<Min>(filter, name, value, incl, ctx, filterCtx);
 }
 
-Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
-                    FilterContext const& filterCtx,
-                    std::shared_ptr<aql::AstNode>&& node) {
-  if (!filter) {
-    return {};
-  }
-
-  // non-deterministic condition or self-referenced variable
-  if (!node->isDeterministic() || arangodb::iresearch::findReference(*node, *ctx.ref)) {
-    // not supported by IResearch, but could be handled by ArangoDB
-    appendExpression(*filter, std::move(node), ctx, filterCtx);
-    return {};
-  }
-
-  bool result;
-
-  if (node->isConstant()) {
-    result = node->isTrue();
-  } else {  // deterministic expression
-    ScopedAqlValue value(*node);
-
-    if (!value.execute(ctx)) {
-      // can't execute expression
-      return {TRI_ERROR_BAD_PARAMETER, "can not execute expression"};
-    }
-
-    result = value.getBoolean();
-  }
-
-  if (result) {
-    filter->add<irs::all>().boost(filterCtx.boost);
-  } else {
-    filter->add<irs::empty>();
-  }
-
-  return {};
-}
-
 Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
                       FilterContext const& filterCtx, aql::AstNode const& node) {
   if (!filter) {
@@ -966,6 +928,15 @@ Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
   return {};
 }
 
+Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
+                    FilterContext const& filterCtx,
+                    std::shared_ptr<aql::AstNode>&& node) {
+  // redirect to existing function for AstNode const& nodes to
+  // avoid coding the logic twice
+  return fromExpression(filter, ctx, filterCtx, *node);
+}
+
+
 // GEO_IN_RANGE(attribute, shape, lower, upper[, includeLower = true, includeUpper = true])
 Result fromFuncGeoInRange(
     char const* funcName,

tests/IResearch/IResearchFilterFunction-test.cpp

@@ -6328,7 +6328,7 @@ TEST_F(IResearchFilterFunctionTest, levenshteinMatch) {
   assertFilterFail(
       vocbase(),
       "FOR d IN myView FILTER LEVENSHTEIN_MATCH(d.foo, 'foo', 5, false) RETURN d");
-  assertFilterExecutionFail(
+  assertFilterFail(
       vocbase(),
       "FOR d IN myView FILTER LEVENSHTEIN_MATCH(d.foo, 'foo', -1, false) RETURN d",
       &ExpressionContextMock::EMPTY);

tests/js/server/aql/aql-options-verification.js

@@ -1,5 +1,5 @@
 /*jshint globalstrict:false, strict:false, maxlen: 500 */
-/*global assertEqual, assertTrue, assertMatch, AQL_EXPLAIN */
+/*global assertEqual, assertTrue, assertMatch, fail, AQL_EXPLAIN */
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief tests for invalid OPTIONS attributes
@@ -80,6 +80,8 @@ function aqlOptionsVerificationSuite () {
         [ prefix + "{ indexHint: [] } RETURN 1", "indexHint" ],
         [ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
         [ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
         [ prefix + "{ method: 'hash' } RETURN 1", "method" ],
         [ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
       ];
@@ -106,7 +108,19 @@ function aqlOptionsVerificationSuite () {
         [ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
       ];
 
-      checkQueries("FOR", queries);
+      // arangosearch only likes boolean attributes for its waitForSync value
+      try {
+        AQL_EXPLAIN(prefix + "{ waitForSync: +1 } RETURN 1");
+        fail();
+      } catch (err) {
+        assertEqual(errors.ERROR_BAD_PARAMETER.code, err.errorNum);
+      }
+      try {
+        AQL_EXPLAIN(prefix + "{ waitForSync: -1 } RETURN 1");
+        fail();
+      } catch (err) {
+        assertEqual(errors.ERROR_BAD_PARAMETER.code, err.errorNum);
+      }
     },
 
     testTraversal : function () {
@@ -130,6 +144,8 @@ function aqlOptionsVerificationSuite () {
         [ prefix + "{ bfs: true, order: 'bfs' } RETURN 1", "order" ],
         [ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
         [ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
         [ prefix + "{ method: 'hash' } RETURN 1", "method" ],
         [ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
       ];
@@ -144,7 +160,10 @@ function aqlOptionsVerificationSuite () {
         [ prefix + "{ defaultWeight: 42.5 } RETURN 1" ],
         [ prefix + "{ weightAttribute: false } RETURN 1", "weightAttribute" ],
         [ prefix + "{ defaultWeight: false } RETURN 1", "defaultWeight" ],
+        [ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
         [ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
+        [ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
         [ prefix + "{ method: 'hash' } RETURN 1", "method" ],
         [ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
       ];
@@ -158,7 +177,10 @@ function aqlOptionsVerificationSuite () {
         [ prefix + "{ method: 'sorted' } RETURN x" ],
         [ prefix + "{ method: 'hash' } RETURN x" ],
         [ prefix + "{ method: 'foxx' } RETURN x", "method" ],
+        [ prefix + "{ waitForSync: false } RETURN x", "waitForSync" ],
         [ prefix + "{ waitForSync: true } RETURN x", "waitForSync" ],
+        [ prefix + "{ waitForSync: +1 } RETURN x", "waitForSync" ],
+        [ prefix + "{ waitForSync: -1 } RETURN x", "waitForSync" ],
         [ prefix + "{ tititi: 'piff' } RETURN x", "tititi" ],
       ];
 
@@ -170,6 +192,8 @@ function aqlOptionsVerificationSuite () {
       const queries = [
         [ prefix + "{ waitForSync: false }" ],
         [ prefix + "{ waitForSync: true }" ],
+        [ prefix + "{ waitForSync: +1 }" ],
+        [ prefix + "{ waitForSync: -1 }" ],
         [ prefix + "{ skipDocumentValidation: true }" ],
         [ prefix + "{ keepNull: true }" ],
         [ prefix + "{ mergeObjects: true }" ],
@@ -191,6 +215,8 @@ function aqlOptionsVerificationSuite () {
       const queries = [
         [ prefix + "{ waitForSync: false }" ],
         [ prefix + "{ waitForSync: true }" ],
+        [ prefix + "{ waitForSync: +1 }" ],
+        [ prefix + "{ waitForSync: -1 }" ],
         [ prefix + "{ skipDocumentValidation: true }" ],
         [ prefix + "{ keepNull: true }" ],
         [ prefix + "{ mergeObjects: true }" ],
@@ -212,6 +238,8 @@ function aqlOptionsVerificationSuite () {
       const queries = [
         [ prefix + "{ waitForSync: false }" ],
         [ prefix + "{ waitForSync: true }" ],
+        [ prefix + "{ waitForSync: +1 }" ],
+        [ prefix + "{ waitForSync: -1 }" ],
         [ prefix + "{ skipDocumentValidation: true }" ],
         [ prefix + "{ keepNull: true }" ],
         [ prefix + "{ mergeObjects: true }" ],
@@ -233,6 +261,8 @@ function aqlOptionsVerificationSuite () {
       const queries = [
         [ prefix + "{ waitForSync: false }" ],
         [ prefix + "{ waitForSync: true }" ],
+        [ prefix + "{ waitForSync: +1 }" ],
+        [ prefix + "{ waitForSync: -1 }" ],
         [ prefix + "{ skipDocumentValidation: true }" ],
         [ prefix + "{ keepNull: true }" ],
         [ prefix + "{ mergeObjects: true }" ],
@@ -254,6 +284,8 @@ function aqlOptionsVerificationSuite () {
       const queries = [
         [ prefix + "{ waitForSync: false }" ],
         [ prefix + "{ waitForSync: true }" ],
+        [ prefix + "{ waitForSync: +1 }" ],
+        [ prefix + "{ waitForSync: -1 }" ],
         [ prefix + "{ skipDocumentValidation: true }" ],
         [ prefix + "{ keepNull: true }" ],
         [ prefix + "{ mergeObjects: true }" ],