EQL: Disallow chained comparisons #62567
Conversation
Expressions like `1 = 2 = 3 = 4` or `1 < 2 = 3 >= 4` were treated with leftmost priority: ((1 = 2) = 3) = 4 which can lead to confusing results. Since such expressions don't make so much change for EQL filters we disallow them in the parser to prevent unexpected results from their bad usage. Major DBs like PostgreSQL and Oracle also disallow them in their SQL syntax. (counter example would be MySQL which interprets them as we did before with leftmost priority). Fixes: elastic#61654
|
Pinging @elastic/es-ql (:Query Languages/EQL) |
elasticmachine
added
the
Team:QL (Deprecated)
|
/cc @jrodewig |
| | left=defaultExpression comparisonOperator right=defaultExpression #comparison | ||
| ; | ||
|
|
||
| defaultExpression |
There was a problem hiding this comment.
Please let me know of a better suggestion for this name as I really couldn't come up with one.
There was a problem hiding this comment.
looks like these are all arithmetic, so maybe arithmeticExpression?
There was a problem hiding this comment.
I opt for operators (operatorExpression) since the block contains arithmetic and comparison operators, the latter being logical not arithmetic.
| @@ -221,4 +221,25 @@ public void testInEmptySet() { | |||
| expectThrows(ParsingException.class, "Expected syntax error", | |||
| () -> expr("name in ()")); | |||
| } | |||
|
|
|||
| public void testChainedComparisons() { | |||
There was a problem hiding this comment.
Can you add a test for something like 1 * 2 <= 3 * 4, just to make sure these operators are still playing nicely with precedence for other operators. Even better would be to verify the AST matches this: (1 * 2) <= (3 * 4)
|
Btw the new grammar was also checked with |
| new Neg(null, new Literal(null, 3, DataTypes.INTEGER)), | ||
| new Literal(null, 4, DataTypes.INTEGER)); | ||
|
|
||
| assertEquals(new LessThanOrEqual(null, left, right, UTC), expr(comparison)); |
There was a problem hiding this comment.
could also do this, although it doesn't make assertions on what the underlying trees are, just that they are equivalent
assertEquals(expr("1 * -2 <= -3 * 4"), expr("(1 * -2) <= (-3 * 4)"))
| | left=defaultExpression comparisonOperator right=defaultExpression #comparison | ||
| ; | ||
|
|
||
| defaultExpression |
There was a problem hiding this comment.
I opt for operators (operatorExpression) since the block contains arithmetic and comparison operators, the latter being logical not arithmetic.
| ; | ||
|
|
||
| defaultExpression | ||
| : primaryExpression predicate? #defaultExpressionDefault |
There was a problem hiding this comment.
defaultExpressionDefault -> operatorExpressionDefault
Expressions like `1 = 2 = 3 = 4` or `1 < 2 = 3 >= 4` were treated with leftmost priority: ((1 = 2) = 3) = 4 which can lead to confusing results. Since such expressions don't make so much change for EQL filters we disallow them in the parser to prevent unexpected results from their bad usage. Major DBs like PostgreSQL and Oracle also disallow them in their SQL syntax. (counter example would be MySQL which interprets them as we did before with leftmost priority). Fixes: #61654 (cherry picked from commit 8f94981)
Expressions like
1 = 2 = 3 = 4or1 < 2 = 3 >= 4were treated withleftmost priority: ((1 = 2) = 3) = 4 which can lead to confusing
results. Since such expressions don't make so much change for EQL
filters we disallow them in the parser to prevent unexpected results
from their bad usage.
Major DBs like PostgreSQL and Oracle also disallow them in their SQL
syntax. (counter example would be MySQL which interprets them as we did
before with leftmost priority).
Fixes: #61654