8000 Fixed BTS-408 (#14513) · arangodb/arangodb@14f612f · GitHub
[go: up one dir, main page]
Skip to content
Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 14f612f

Browse files
jsteemannjsteemann
authored
Fixed BTS-408 (#14513)
1 parent 5786402 commit 14f612f

File tree

8 files changed

+77
-65
lines changed

8 files changed

+77
-65
lines changed

‎CHANGELOG

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
devel
22
-----
33

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

‎arangod/Aql/Ast.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,11 @@ class Ast {
492492
/// isValid is set to false, then the returned value is not to be trued and the
493493
/// the result is equivalent to an AQL `null` value
494494
static AstNode const* resolveConstAttributeAccess(AstNode const*, bool& isValid);
495+
496+
/// @brief optimizes the unary operators + and -
497+
/// the unary plus will be converted into a simple value node if the operand
498+
/// of the operation is a constant number
499+
AstNode* optimizeUnaryOperatorArithmetic(AstNode*);
495500

496501
private:
497502
/// @brief make condition from example
@@ -503,11 +508,6 @@ class Ast {
503508
/// @brief create a number node for an arithmetic result, double
504509
AstNode* createArithmeticResultNode(double);
505510

506-
/// @brief optimizes the unary operators + and -
507-
/// the unary plus will be converted into a simple value node if the operand
508-
/// of the operation is a constant number
509-
AstNode* optimizeUnaryOperatorArithmetic(AstNode*);
510-
511511
/// @brief optimizes the unary operator NOT with a non-constant expression
512512
AstNode* optimizeNotExpression(AstNode*);
513513

‎arangod/Aql/grammar.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3951,15 +3951,15 @@ YYLTYPE yylloc = yyloc_default;
39513951
case 127: /* operator_unary: "+ operator" expression */
39523952
#line 1435 "Aql/grammar.y"
39533953
{
3954-
(yyval.node) = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, (yyvsp[0].node));
3954+
(yyval.node) = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OP 6D40 ERATOR_UNARY_PLUS, (yyvsp[0].node)));
39553955
}
39563956
#line 3957 "Aql/grammar.cpp"
39573957
break;
39583958

39593959
case 128: /* operator_unary: "- operator" expression */
39603960
#line 1438 "Aql/grammar.y"
39613961
{
3962-
(yyval.node) = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, (yyvsp[0].node));
3962+
(yyval.node) = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, (yyvsp[0].node)));
39633963
}
39643964
#line 3965 "Aql/grammar.cpp"
39653965
break;

‎arangod/Aql/grammar.y

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,10 +1433,10 @@ function_call:
14331433

14341434
operator_unary:
14351435
T_PLUS expression %prec UPLUS {
1436-
$$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, $2);
1436+
$$ = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_PLUS, $2));
14371437
}
14381438
| T_MINUS expression %prec UMINUS {
1439-
$$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, $2);
1439+
$$ = parser->ast()->optimizeUnaryOperatorArithmetic(parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_MINUS, $2));
14401440
}
14411441
| T_NOT expression %prec UNEGATION {
14421442
$$ = parser->ast()->createNodeUnaryOperator(NODE_TYPE_OPERATOR_UNARY_NOT, $2);

‎arangod/IResearch/IResearchFilterFactory.cpp

Lines changed: 9 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -891,44 +891,6 @@ Result byRange(irs::boolean_filter* filter,
891891
return byRange<Min>(filter, name, value, incl, ctx, filterCtx);
892892
}
893893

894-
Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
895-
FilterContext const& filterCtx,
896-
std::shared_ptr<aql::AstNode>&& node) {
897-
if (!filter) {
898-
return {};
899-
}
900-
901-
// non-deterministic condition or self-referenced variable
902-
if (!node->isDeterministic() || arangodb::iresearch::findReference(*node, *ctx.ref)) {
903-
// not supported by IResearch, but could be handled by ArangoDB
904-
appendExpression(*filter, std::move(node), ctx, filterCtx);
905-
return {};
906-
}
907-
908-
bool result;
909-
910-
if (node->isConstant()) {
911-
result = node->isTrue();
912-
} else { // deterministic expression
913-
ScopedAqlValue value(*node);
914-
915-
if (!value.execute(ctx)) {
916-
// can't execute expression
917-
return {TRI_ERROR_BAD_PARAMETER, "can not execute expression"};
918-
}
919-
920-
result = value.getBoolean();
921-
}
922-
923-
if (result) {
924-
filter->add<irs::all>().boost(filterCtx.boost);
925-
} else {
926-
filter->add<irs::empty>();
927-
}
928-
929-
return {};
930-
}
931-
932894
Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
933895
FilterContext const& filterCtx, aql::AstNode const& node) {
934896
if (!filter) {
@@ -966,6 +928,15 @@ Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
966928
return {};
967929
}
968930

931+
Result fromExpression(irs::boolean_filter* filter, QueryContext const& ctx,
932+
FilterContext const& filterCtx,
933+
std::shared_ptr<aql::AstNode>&& node) {
934+
// redirect to existing function for AstNode const& nodes to
935+
// avoid coding the logic twice
936+
return fromExpression(filter, ctx, filterCtx, *node);
937+
}
938+
939+
969940
// GEO_IN_RANGE(attribute, shape, lower, upper[, includeLower = true, includeUpper = true])
970941
Result fromFuncGeoInRange(
971942
char const* funcName,

‎tests/IResearch/IResearchFilterFunction-test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6328,7 +6328,7 @@ TEST_F(IResearchFilterFunctionTest, levenshteinMatch) {
63286328
assertFilterFail(
63296329
vocbase(),
63306330
"FOR d IN myView FILTER LEVENSHTEIN_MATCH(d.foo, 'foo', 5, false) RETURN d");
6331-
assertFilterExecutionFail(
6331+
assertFilterFail(
63326332
vocbase(),
63336333
"FOR d IN myView FILTER LEVENSHTEIN_MATCH(d.foo, 'foo', -1, false) RETURN d",
63346334
&ExpressionContextMock::EMPTY);

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

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*jshint globalstrict:false, strict:false, maxlen: 500 */
2-
/*global assertEqual, assertTrue, assertMatch, AQL_EXPLAIN */
2+
/*global assertEqual, assertTrue, assertMatch, fail, AQL_EXPLAIN */
33

44
////////////////////////////////////////////////////////////////////////////////
55
/// @brief tests for invalid OPTIONS attributes
@@ -80,6 +80,8 @@ function aqlOptionsVerificationSuite () {
8080
[ prefix + "{ indexHint: [] } RETURN 1", "indexHint" ],
8181
[ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
8282
[ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
83+
[ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
84+
[ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
8385
[ prefix + "{ method: 'hash' } RETURN 1", "method" ],
8486
[ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
8587
];
@@ -106,7 +108,19 @@ function aqlOptionsVerificationSuite () {
106108
[ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
107109
];
108110

109-
checkQueries("FOR", queries);
111+
// arangosearch only likes boolean attributes for its waitForSync value
112+
try {
113+
AQL_EXPLAIN(prefix + "{ waitForSync: +1 } RETURN 1");
114+
fail();
115+
} catch (err) {
116+
assertEqual(errors.ERROR_BAD_PARAMETER.code, err.errorNum);
117+
}
118+
try {
119+
AQL_EXPLAIN(prefix + "{ waitForSync: -1 } RETURN 1");
120+
fail();
121+
} catch (err) {
122+
assertEqual(errors.ERROR_BAD_PARAMETER.code, err.errorNum);
123+
}
110124
},
111125

112126
testTraversal : function () {
@@ -130,6 +144,8 @@ function aqlOptionsVerificationSuite () {
130144
[ prefix + "{ bfs: true, order: 'bfs' } RETURN 1", "order" ],
131145
[ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
132146
[ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
147+
[ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
148+
[ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
133149
[ prefix + "{ method: 'hash' } RETURN 1", "method" ],
134150
[ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
135151
];
@@ -144,7 +160,10 @@ function aqlOptionsVerificationSuite () {
144160
[ prefix + "{ defaultWeight: 42.5 } RETURN 1" ],
145161
[ prefix + "{ weightAttribute: false } RETURN 1", "weightAttribute" ],
146162
[ prefix + "{ defaultWeight: false } RETURN 1", "defaultWeight" ],
163+
[ prefix + "{ waitForSync: false } RETURN 1", "waitForSync" ],
147164
[ prefix + "{ waitForSync: true } RETURN 1", "waitForSync" ],
165+
[ prefix + "{ waitForSync: +1 } RETURN 1", "waitForSync" ],
166+
[ prefix + "{ waitForSync: -1 } RETURN 1", "waitForSync" ],
148167
[ prefix + "{ method: 'hash' } RETURN 1", "method" ],
149168
[ prefix + "{ tititi: 'piff' } RETURN 1", "tititi" ],
150169
];
@@ -158,7 +177,10 @@ function aqlOptionsVerificationSuite () {
158177
[ prefix + "{ method: 'sorted' } RETURN x" ],
159178
[ prefix + "{ method: 'hash' } RETURN x" ],
160179
[ prefix + "{ method: 'foxx' } RETURN x", "method" ],
180+
[ prefix + "{ waitForSync: false } RETURN x", "waitForSync" ],
161181
[ prefix + "{ waitForSync: true } RETURN x", "waitForSync" ],
182+
[ prefix + "{ waitForSync: +1 } RETURN x", "waitForSync" ],
183+
[ prefix + "{ waitForSync: -1 F438 } RETURN x", "waitForSync" ],
162184
[ prefix + "{ tititi: 'piff' } RETURN x", "tititi" ],
163185
];
164186

@@ -170,6 +192,8 @@ function aqlOptionsVerificationSuite () {
170192
const queries = [
171193
[ prefix + "{ waitForSync: false }" ],
172194
[ prefix + "{ waitForSync: true }" ],
195+
[ prefix + "{ waitForSync: +1 }" ],
196+
[ prefix + "{ waitForSync: -1 }" ],
173197
[ prefix + "{ skipDocumentValidation: true }" ],
174198
[ prefix + "{ keepNull: true }" ],
175199
[ prefix + "{ mergeObjects: true }" ],
@@ -191,6 +215,8 @@ function aqlOptionsVerificationSuite () {
191215
const queries = [
192216
[ prefix + "{ waitForSync: false }" ],
193217
[ prefix + "{ waitForSync: true }" ],
218+
[ prefix + "{ waitForSync: +1 }" ],
219+
[ prefix + "{ waitForSync: -1 }" ],
194220
[ prefix + "{ skipDocumentValidation: true }" ],
195221
[ prefix + "{ keepNull: true }" ],
196222
[ prefix + "{ mergeObjects: true }" ],
@@ -212,6 +238,8 @@ function aqlOptionsVerificationSuite () {
212238
const queries = [
213239
[ prefix + "{ waitForSync: false }" ],
214240
[ prefix + "{ waitForSync: true }" ],
241+
[ prefix + "{ waitForSync: +1 }" ],
242+
[ prefix + "{ waitForSync: -1 }" ],
215243
[ prefix + "{ skipDocumentValidation: true }" ],
216244
[ prefix + "{ keepNull: true }" ],
217245
[ prefix + "{ mergeObjects: true }" ],
@@ -233,6 +261,8 @@ function aqlOptionsVerificationSuite () {
233261
const queries = [
234262
[ prefix + "{ waitForSync: false }" ],
235263
[ prefix + "{ waitForSync: true }" ],
264+
[ prefix + "{ waitForSync: +1 }" ],
265+
[ prefix + "{ waitForSync: -1 }" ],
236266
[ prefix + "{ skipDocumentValidation: true }" ],
237267
[ prefix + "{ keepNull: true }" ],
238268
[ prefix + "{ mergeObjects: true }" ],
@@ -254,6 +284,8 @@ function aqlOptionsVerificationSuite () {
254284
const queries = [
255285
[ prefix + "{ waitForSync: false }" ],
256286
[ prefix + "{ waitForSync: true }" ],
287+
[ prefix + "{ waitForSync: +1 }" ],
288+
[ prefix + "{ waitForSync: -1 }" ],
257289
[ prefix + "{ skipDocumentValidation: true }" ],
258290
[ prefix + "{ keepNull: true }" ],
259291
[ prefix + "{ mergeObjects: true }" ],

‎tests/js/server/aql/aql-parse.js

Lines changed: 15 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
0