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

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_OPERATOR_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 65CE " ],
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 } 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