From 8702423490ffaf4e27a20860722251a007305436 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Fri, 16 Jul 2021 21:42:21 +0200 Subject: [PATCH 1/4] 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. --- CHANGELOG | 9 +++++ arangod/Aql/Ast.h | 10 +++--- arangod/Aql/grammar.cpp | 4 +-- arangod/Aql/grammar.y | 4 +-- .../js/server/aql/aql-options-verification.js | 36 +++++++++++++++++-- 5 files changed, 52 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d06bb8bdb64a..40087c3c84db 100644 --- a/CHANGELOG +++ b/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, diff --git a/arangod/Aql/Ast.h b/arangod/Aql/Ast.h index b5c94a69273c..69fdf292401d 100644 --- a/arangod/Aql/Ast.h +++ b/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*); diff --git a/arangod/Aql/grammar.cpp b/arangod/Aql/grammar.cpp index f012d383f1c0..f7b93bb0f64e 100644 --- a/arangod/Aql/grammar.cpp +++ b/arangod/Aql/grammar.cpp @@ -3951,7 +3951,7 @@ 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; @@ -3959,7 +3959,7 @@ YYLTYPE yylloc = yyloc_default; 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; diff --git a/arangod/Aql/grammar.y b/arangod/Aql/grammar.y index 3de9903054d0..1da462e716e9 100644 --- a/arangod/Aql/grammar.y +++ b/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); diff --git a/tests/js/server/aql/aql-options-verification.js b/tests/js/server/aql/aql-options-verification.js index 8f0f5258e241..c74f13ed61ba 100644 --- a/tests/js/server/aql/aql-options-verification.js +++ b/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 }" ], From f48089d4afc06899bb562f874825c85d171b0ee9 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Mon, 19 Jul 2021 10:54:12 +0200 Subject: [PATCH 2/4] fix test --- tests/js/server/aql/aql-parse.js | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/js/server/aql/aql-parse.js b/tests/js/server/aql/aql-parse.js index 60e2fb4ac624..5de277182005 100644 --- a/tests/js/server/aql/aql-parse.js +++ b/tests/js/server/aql/aql-parse.js @@ -248,33 +248,33 @@ function ahuacatlParseTestSuite () { returnNode = getRoot("return -1").subNodes[0]; assertEqual("return", returnNode.type); - assertEqual("unary minus", returnNode.subNodes[0].type); - assertEqual("value", returnNode.subNodes[0].subNodes[0].type); - assertEqual(1, returnNode.subNodes[0].subNodes[0].value); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(-1, returnNode.subNodes[0].value); returnNode = getRoot("return -193817872892").subNodes[0]; assertEqual("return", returnNode.type); - assertEqual("unary minus", returnNode.subNodes[0].type); - assertEqual("value", returnNode.subNodes[0].subNodes[0].type); - assertEqual(193817872892, returnNode.subNodes[0].subNodes[0].value); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(-193817872892, returnNode.subNodes[0].value); returnNode = getRoot("return -.95264").subNodes[0]; assertEqual("return", returnNode.type); - assertEqual("unary minus", returnNode.subNodes[0].type); - assertEqual("value", returnNode.subNodes[0].subNodes[0].type); - assertEqual(0.95264, returnNode.subNodes[0].subNodes[0].value); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(-0.95264, returnNode.subNodes[0].value); returnNode = getRoot("return -12345").subNodes[0]; assertEqual("return", returnNode.type); - assertEqual("unary minus", returnNode.subNodes[0].type); - assertEqual("value", returnNode.subNodes[0].subNodes[0].type); - assertEqual(12345, returnNode.subNodes[0].subNodes[0].value); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(-12345, returnNode.subNodes[0].value); returnNode = getRoot("return -92.4987521").subNodes[0]; assertEqual("return", returnNode.type); - assertEqual("unary minus", returnNode.subNodes[0].type); - assertEqual("value", returnNode.subNodes[0].subNodes[0].type); - assertEqual(92.4987521, returnNode.subNodes[0].subNodes[0].value); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(-92.4987521, returnNode.subNodes[0].value); + + returnNode = getRoot("return +1234.56").subNodes[0]; + assertEqual("return", returnNode.type); + assertEqual("value", returnNode.subNodes[0].type); + assertEqual(1234.56, returnNode.subNodes[0].value); }, //////////////////////////////////////////////////////////////////////////////// From 6b51c70daba1f38da038569a6cd8edefa46da0f5 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Mon, 19 Jul 2021 12:07:31 +0200 Subject: [PATCH 3/4] deduplicate code --- arangod/IResearch/IResearchFilterFactory.cpp | 47 ++++---------------- 1 file changed, 9 insertions(+), 38 deletions(-) diff --git a/arangod/IResearch/IResearchFilterFactory.cpp b/arangod/IResearch/IResearchFilterFactory.cpp index 9c7e78ee3b24..e2f9e70c2f91 100644 --- a/arangod/IResearch/IResearchFilterFactory.cpp +++ b/arangod/IResearch/IResearchFilterFactory.cpp @@ -891,44 +891,6 @@ Result byRange(irs::boolean_filter* filter, return byRange(filter, name, value, incl, ctx, filterCtx); } -Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx, - FilterContext const& filterCtx, - std::shared_ptr&& 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().boost(filterCtx.boost); - } else { - filter->add(); - } - - 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&& 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, From 1ced7e6acd7f21d1c03e8bda8adcda0671f67e4c Mon Sep 17 00:00:00 2001 From: jsteemann Date: Mon, 19 Jul 2021 12:07:42 +0200 Subject: [PATCH 4/4] adjust test expectation --- tests/IResearch/IResearchFilterFunction-test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/IResearch/IResearchFilterFunction-test.cpp b/tests/IResearch/IResearchFilterFunction-test.cpp index aeace80aa083..b12e2c26906d 100644 --- a/tests/IResearch/IResearchFilterFunction-test.cpp +++ b/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);