8000 Fixed BTS-408 by jsteemann · Pull Request #14513 · arangodb/arangodb · GitHub
[go: up one dir, main page]
Skip to content

Fixed BTS-408 #14513

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 5 commits into from
Jul 19, 2021
Merged

Fixed BTS-408 #14513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8702423
Fixed BTS-408
jsteemann Jul 16, 2021
e80f9e6
Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/bts…
jsteemann Jul 19, 2021
f48089d
fix test
jsteemann Jul 19, 2021
6b51c70
deduplicate code
jsteemann Jul 19, 2021
1ced7e6
adjust test expectation
jsteemann Jul 19, 2021
File filter

Filter by extension

Filter by extension
10000 Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
10 changes: 5 additions & 5 deletions arangod/Aql/Ast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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*);

Expand Down
4 changes: 2 additions & 2 deletions arangod/Aql/grammar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions arangod/Aql/grammar.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
47 changes: 9 additions & 38 deletions arangod/IResearch/IResearchFilterFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion tests/IResearch/IResearchFilterFunction-test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
36 changes: 34 additions & 2 deletions tests/js/server/aql/aql-options-verification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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" ],
];
Expand All @@ -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 () {
Expand All @@ -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" ],
];
Expand All @@ -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" ],
];
Expand All @@ -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" ],
];

Expand All @@ -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 }" ],
Expand All @@ -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 }" ],
Expand All @@ -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 }" ],
Expand All @@ -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 }" ],
Expand All @@ -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 }" ],
Expand Down
30 changes: 15 additions & 15 deletions tests/js/server/aql/aql-parse.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0