QL: Simplify arithmetic operations in binary comps #66022

bpintea · 2020-12-08T13:04:34Z

This PR adds an optimizer rule to simplify the arithmetic operations
in binary comparison expressions, which in turn will allow for further
expression compounding by the optimiser.

Only the negation and plus, minus, multiplication and division are
currently considered and only when two of the operands are a literal.

For instance ((a + 1) / 2 - 3) * 4 >= 14 becomes a >= 12.

Closes #65394.

This commit adds an optimizer rule to simplify the arithmetic operations in binary comparison expressions, which in turn will allow for further expression compounding by the optimiser. Only the negation and plus, minus, multiplication and division are currently considered and only when two of the operands are a literal. For instance `((a + 1) / 2 - 3) * 4 >= 14` becomes `a >= 12`.

Revert unintentional change of a source file.

elasticmachine · 2020-12-08T13:04:38Z

Pinging @elastic/es-ql (Team:QL)

astefan

This PR is changing one behavior that might not be so obvious at first: a query like SELECT salary FROM test_emp WHERE salary + 10 >= 'x' returns no documents in master, but throws Cannot compute [-] between [String] [Integer] in this PR.

Apart from the fact that the error message is slightly incomplete - missing and between [String] and [Integer] - it's a confusing error message for the user. The minus sign is not present in the query, but present in the error message.

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

costin

I'd like to see more tests including non-numeric values considering the binary operators inside SQL support intervals and dates.
The cast to double is problematic - what does 4 / 2 return vs 5 / 2? And why is the double casting having to be applied manually when we do not do that for / in the first place.
What happens in case of :

date - INTERVAL 1 MONTH > '2010-01-01T01:01:01` ?

...va/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mul.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

palesz

I have one major concern that I would like to discuss before moving ahead with the PR.

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java

The optimisation is now refactor splitting the logic by add-sub and mul-div operations. Further restictions are put in place, to prevent the optimiser from chaning the outcome of the filtering. More tests have been added.

bpintea · 2020-12-14T09:17:10Z

@elasticmachine update branch

Removed now unused import.

- update test condition to remove (non-optimisable) integer division.

bpintea · 2020-12-14T10:35:55Z

This PR is changing one behavior that might not be so obvious at first: a query like SELECT salary FROM test_emp WHERE salary + 10 >= 'x' returns no documents in master, but throws Cannot compute [-] between [String] [Integer] in this PR.

Thanks, I was wrongly expecting that this check had already been in place (but #49636 is still open).

Apart from the fact that the error message is slightly incomplete - missing and between [String] and [Integer] - it's a confusing error message for the user. The minus sign is not present in the query, but present in the error message.

This hasn't been reported before, so I went ahead and fixed it part of this PR.

palesz

Halfway through, few comments and suggestions so far.

...org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java

costin

Looks to me like this PR is made of two distinct parts.

the 'bubbling' of negations (which I would rename to SimplifyDoubleNegations).
The actual simplification of arithmetic in comparisons.

2 depends on 1 however 1 has value by itself and it would be easier to reason about stand-alone.

x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt

...org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java

...l/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Add.java

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

- add isMulOrDiv and isAddOrSub methods; - add a new factory if'ace for the arithmetic operations invertible in relation to a binary comparison; - simplify NEG - NEG tree structures; - more tests.

…hm_in_bc

bpintea · 2021-01-07T17:00:33Z

the 'bubbling' of negations (which I would rename to SimplifyDoubleNegations).

I've converted the PR back to draft to extract the rule on negations. However, I assume the rule will mostly extract the negation up (i.e. bubbling them up: DIV/MUL - NEG - x => NEG - DIV/MUL - x), rather then reduce them (i.e. NEG - NEG - x => x), so not sure if "Simplify" would be correct.

costin · 2021-01-14T21:15:37Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+    public static final class SimplifyComparisonsArithmetics extends OptimizerExpressionRule {
+        BiFunction<DataType, DataType, Boolean> typesCompatible;
+
+        public SimplifyComparisonsArithmetics(BiFunction<DataType, DataType, Boolean> typesCompatible) {


typesCompatible -> typeCheck

Before I change it, just to 2x check: the passed arguments are DataTypes::areCompatible and SqlDataTypes::areCompatible.
The code using the var also looks like return typesCompatible.apply([type1], [type2]) == false;.
Not sure if the change would bring more clarity ("what's checked?"), so asking.

astefan

Left some comments

astefan · 2021-01-19T08:00:46Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+            return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg));
+        }
+
+        static Expression safeMaybeFold(Expression expression) {


I don't particularly like this method and, imo, it doesn't help much understanding the code. I think the return null inside the catch block bothers me.
I would have kept the original (no separate method used) block of code, as I find it easier to read.

private static Expression reduceNegation(BinaryComparison bc) { Literal bcLiteral = (Literal) bc.right(); Expression expression = new Neg(bcLiteral.source(), bcLiteral); if (expression.foldable()) { try { expression = new Literal(expression.source(), expression.fold(), expression.dataType()); return bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), expression)); } catch (ArithmeticException | DateTimeException e) { // implicitly return the original BinaryComparison in case the folding results in scenarios where, // for example, we have Long.MAX_VALUE+1 to fold. In this case, we skip the optimization. } } return bc; }

I think the return null inside the catch block bothers me.

The null isn't returned from the catch block, it's just assigned to the var to return.
But this still being a detail, would you mind expanding what's troubling / how it could be improved?

I would have kept the original (no separate method used)

The code is extracted as a method since it's reused. It should be acceptably concise too: "if the expression is foldable, attempt to do so. Return either folded outcome, original (if not foldable) or null (if foldable, but unsafely; i.e. exception wud be thrown)".
An alternative could be for the caller to save the input expression (as in your example), the function to return the original input, if folding is unsafe, and for caller to check returned value against the original input. But this would be more verbose (as in your example) and this pattern to be applied multiple times.

For now I've taken Costin's suggestion, but still open for change if needed.

Can be left as is I guess. My initial suggestion seems to be just a personal preference. If there will be other changes to this PR, I'd add a comment similar to the one I made in the catch (ArithmeticException | DateTimeException e) { to explain the catch block "ignoring" the math failure.

astefan · 2021-01-19T08:05:52Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+            if (bc.right() instanceof Literal) {
+                if (bc.left() instanceof ArithmeticOperation) {
+                    return SimplifyOperation.simplify(bc, typesCompatible);
+                } else if (bc.left() instanceof Neg) {


I think only if (bc.left().... is enough. else if doesn't necessarily help here.

I've changed it.

(Particularly here, the if[-else] is practically a switch, on a type that's not switchable in Java, so the change makes sense.
Generally I'm in favour of the if-else format when:

a return is involved, whose later replacement by a non-jumping statement, could make the next if branch eligible; i.e. less error-prone.

is more compact.)

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec

...csearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticOperation.java

…hm_in_bc

- rename methods for more clarify; - regroup simplification checks; - eliminate calling super class' methods; - use == for static string comps; - use Math.signum instead of inlining the calculation.

bpintea · 2021-01-27T10:50:23Z

@elasticmachine update branch

Update QA test.

…hm_in_bc

astefan

LGTM

astefan · 2021-01-28T08:42:20Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+            return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg));
+        }
+
+        static Expression safeMaybeFold(Expression expression) {


Can be left as is I guess. My initial suggestion seems to be just a personal preference. If there will be other changes to this PR, I'd add a comment similar to the one I made in the catch (ArithmeticException | DateTimeException e) { to explain the catch block "ignoring" the math failure.

costin

LGTM. Left some stylist comments, no need for another review from me.

costin · 2021-01-28T13:17:06Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+            return (simplification == null || simplification.isUnsafe(typesCompatible)) ? comparison : simplification.apply();
+        }
+
+        private static boolean isMulOrDiv(String opSymbol) {


If this method necessary? I only see it used in one place and the check can check can be directly just life for ADD and SUB.

It's being used twice. I'm on the fence tbh - it makes things easier to read in sign(), but looks unbalanced in the if-else explicit ADD/SUB check - but would leave it as is now.

costin · 2021-01-28T13:19:46Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

-            static boolean isMulOrDiv(String opSymbol) {
-                return MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol);
+            //  - post adjustments
+            Expression opSpecificPostApply(BinaryComparison binaryComparison) {


postApply or postProcess. opSpecific is somewhat meaningless.

I went for postProcess.

costin · 2021-01-28T13:20:43Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

@@ -1221,7 +1245,7 @@ static Expression safeMaybeFold(Expression expression) {
            }

            // can it be quickly fast-tracked that the operation can't be reduced?
-            boolean cannotSimplify(BiFunction<DataType, DataType, Boolean> typesCompatible) {
+            boolean isUnsafe(BiFunction<DataType, DataType, Boolean> typesCompatible) {


If you don't this to be subclass because of the dedicated method, consider making it final.

Right, thanks, applied! Applicable to both class' main methods.

costin · 2021-01-28T13:22:09Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java

+                    return true;
+                }
+
+                return opSpecificIsUnsafe();


how about check, doCheck without opSpecific?

- rename methods; - add comments.

…hm_in_bc

Both upstream and current branch updated line in the same way (incrementing a count), so merging left it unchanged. However the net result needs to reflect both modifications (i.e. inc by two).

bpintea · 2021-02-01T17:35:20Z

@elasticmachine update branch

* Simplify arithmetic operations in binary comps This commit adds an optimizer rule to simplify the arithmetic operations in binary comparison expressions, which in turn will allow for further expression compounding by the optimiser. Only the negation and plus, minus, multiplication and division are currently considered and only when two of the operands are a literal. For instance `((a + 1) / 2 - 3) * 4 >= 14` becomes `a >= 12`. (cherry picked from commit f5c2982)

… (#68331) * QL: Simplify arithmetic operations in binary comps (#66022) * Simplify arithmetic operations in binary comps This commit adds an optimizer rule to simplify the arithmetic operations in binary comparison expressions, which in turn will allow for further expression compounding by the optimiser. Only the negation and plus, minus, multiplication and division are currently considered and only when two of the operands are a literal. For instance `((a + 1) / 2 - 3) * 4 >= 14` becomes `a >= 12`. (cherry picked from commit f5c2982)

bpintea added 2 commits December 8, 2020 13:47

Undo unintentional change

0db90e4

Revert unintentional change of a source file.

bpintea added >enhancement :Analytics/SQL SQL querying v8.0.0 :Analytics/EQL EQL querying v7.11.0 labels Dec 8, 2020

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Dec 8, 2020

astefan requested changes Dec 8, 2020

View reviewed changes

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java Outdated Show resolved Hide resolved

costin requested changes Dec 8, 2020

View reviewed changes

costin requested a review from palesz December 8, 2020 14:58

palesz suggested changes Dec 8, 2020

View reviewed changes

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java Outdated Show resolved Hide resolved

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java Outdated Show resolved Hide resolved

bpintea marked this pull request as draft December 9, 2020 09:10

Refactor the optimisation. Adress review comments

abc632d

The optimisation is now refactor splitting the logic by add-sub and mul-div operations. Further restictions are put in place, to prevent the optimiser from chaning the outcome of the filtering. More tests have been added.

elasticmachine and others added 3 commits December 14, 2020 01:17

Merge branch 'master' into enh/optimize_arithm_in_bc

b031dfb

Style fix

4d641e4

Removed now unused import.

Test fix

1a28179

- update test condition to remove (non-optimisable) integer division.

bpintea marked this pull request as ready for review December 14, 2020 11:41

bpintea mentioned this pull request Dec 15, 2020

SQL: enhance the test data set with more data types #66325

Closed

palesz reviewed Dec 18, 2020

View reviewed changes

pugnascotia added v7.12.0 and removed v7.11.0 labels Jan 5, 2021

costin requested changes Jan 5, 2021

View reviewed changes

bpintea added 2 commits January 7, 2021 16:42

Address review comments

e74b697

- add isMulOrDiv and isAddOrSub methods; - add a new factory if'ace for the arithmetic operations invertible in relation to a binary comparison; - simplify NEG - NEG tree structures; - more tests.

Merge remote-tracking branch 'upstream/master' into enh/optimize_arit…

25becef

…hm_in_bc

bpintea marked this pull request as draft January 7, 2021 16:56

costin reviewed Jan 14, 2021

View reviewed changes

astefan reviewed Jan 19, 2021

View reviewed changes

bpintea added 2 commits January 26, 2021 17:10

Merge remote-tracking branch 'upstream/master' into enh/optimize_arit…

6dadbeb

…hm_in_bc

Address feedback

70df981

- rename methods for more clarify; - regroup simplification checks; - eliminate calling super class' methods; - use == for static string comps; - use Math.signum instead of inlining the calculation.

elasticmachine and others added 3 commits January 27, 2021 02:50

Merge branch 'master' into enh/optimize_arithm_in_bc

f74d627

Address review comments

99f08c1

Update QA test.

Merge remote-tracking branch 'upstream/master' into enh/optimize_arit…

a3b66c8

…hm_in_bc

bpintea requested review from costin and astefan January 27, 2021 12:05

astefan approved these changes Jan 28, 2021

View reviewed changes

costin approved these changes Jan 28, 2021

View reviewed changes

bpintea added 4 commits January 28, 2021 20:21

Address review comments

8eee4ea

- rename methods; - add comments.

Merge remote-tracking branch 'upstream/master' into enh/optimize_arit…

053fcde

…hm_in_bc

Merge remote-tracking branch 'upstream/master' into enh/optimize_arit…

1c7781f

…hm_in_bc

Adjust merging outcome for one test

28ba3e5

Both upstream and current branch updated line in the same way (incrementing a count), so merging left it unchanged. However the net result needs to reflect both modifications (i.e. inc by two).

Merge branch 'master' into enh/optimize_arithm_in_bc

18c11be

bpintea merged commit f5c2982 into elastic:master Feb 1, 2021

bpintea deleted the enh/optimize_arithm_in_bc branch February 1, 2021 19:09

bpintea added the backport pending label Feb 1, 2021

bpintea removed the backport pending label Feb 2, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

bpintea mentioned this pull request Aug 17, 2021

EQL: Backwards compatibility support for multi value fields update #76605

Merged

astefan mentioned this pull request May 2, 2024

[ESQL] Migrate SimplifiyComparisonArithmetics optimization rule #108200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QL: Simplify arithmetic operations in binary comps #66022

QL: Simplify arithmetic operations in binary comps #66022

bpintea commented Dec 8, 2020 •

edited

Loading

elasticmachine commented Dec 8, 2020

astefan left a comment

costin left a comment •

edited

Loading

palesz left a comment

bpintea commented Dec 14, 2020

bpintea commented Dec 14, 2020

palesz left a comment

costin left a comment

bpintea commented Jan 7, 2021

costin Jan 14, 2021

bpintea Jan 27, 2021

astefan left a comment

astefan Jan 19, 2021

bpintea Jan 27, 2021

astefan Jan 28, 2021

astefan Jan 19, 2021

bpintea Jan 27, 2021

bpintea commented Jan 27, 2021

astefan left a comment

astefan Jan 28, 2021

costin left a comment

costin Jan 28, 2021

bpintea Jan 28, 2021

costin Jan 28, 2021

bpintea Jan 28, 2021

costin Jan 28, 2021

bpintea Jan 28, 2021

costin Jan 28, 2021

bpintea Jan 28, 2021

bpintea commented Feb 1, 2021

QL: Simplify arithmetic operations in binary comps #66022

QL: Simplify arithmetic operations in binary comps #66022

Conversation

bpintea commented Dec 8, 2020 • edited Loading

elasticmachine commented Dec 8, 2020

astefan left a comment

Choose a reason for hiding this comment

costin left a comment • edited Loading

Choose a reason for hiding this comment

palesz left a comment

Choose a reason for hiding this comment

bpintea commented Dec 14, 2020

bpintea commented Dec 14, 2020

palesz left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

bpintea commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Jan 27, 2021

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Feb 1, 2021

bpintea commented Dec 8, 2020 •

edited

Loading

costin left a comment •

edited

Loading