SQL: Optimisation fixes for conjunction merges #50703

bpintea · 2020-01-07T14:14:37Z

This PR fixes the following issues around the way comparisions are
merged with ranges in conjunctions:

the decision to include the equality of the lower limit is corrected;
the selection of the upper limit is corrected to use the upper bound
of the range;
the list of terms in the conjunction is sorted to have the ranges at
the bottom; this allows subsequent binary comarisions to find compatible
ranges and potentially be merged away. The end guarantee being that the
optimisation takes place irrespective of the order of the conjunction
terms in the statement.

Some comments are also corrected.

Addresses #49637

This commit fixes the following issues around the way comparisions are merged with ranges in conjunctions: * the decision to include the equality of the lower limit is corrected; * the selection of the upper limit is corrected to use the upper bound of the range; * the list of terms in the conjunction is sorted to have the ranges at the bottom; this allows subsequent binary comarisions to find compatible ranges and potentially be merged away. The end guarantee being that the optimisation takes place irrespective of the order of the conjunction terms in the statement. Some comments are also corrected.

elasticmachine · 2020-01-07T14:14:40Z

Pinging @elastic/es-search (:Search/SQL)

costin

LGTM.
Thanks for catching the bug while at it - can you please double check all cases are covered by the existing test cases?

costin · 2020-01-07T15:38:37Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -1316,7 +1331,7 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List<Ran
                                    ranges.remove(i);
                                    ranges.add(i,
                                            new Range(other.source(), other.value(),
-                                                    main.right(), lowerEq ? true : other.includeLower(),
+                                                    main.right(), lowerEq ? false : main instanceof GreaterThanOrEqual,


That's a big miss - were there no tests for this?

It seemed much like a c&p fallout.
There wasn't a test, no, so I've simply implemented the test listed as comment in the source (2 < a AND (2 <= a < 3) -> 2 < a < 3 / testCombineBinaryComparisonsAndRangeLower())

costin · 2020-01-07T15:39:01Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -1325,19 +1340,19 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List<Ran
                            }
                        }
                    } else if (main instanceof LessThan || main instanceof LessThanOrEqual) {
-                        if (other.lower().foldable()) {
-                            Integer comp = BinaryComparison.compare(value, other.lower().fold());
+                        if (other.upper().foldable()) {


Likewise - a whole optimization being skipped...

bpintea · 2020-01-08T09:10:04Z

Thanks for catching the bug while at it - can you please double check all cases are covered by the existing test cases?

I hope the coverage should be good now.
There are however some further tunings possible. For instance smth like (1 < a < 3) OR (2 < a < 4) -> (1 < a < 4) isn't optimised, or (1 < a < 2) AND (3 < a < 4) results into 3 < a < 2, which will eventually still yield the correct result, but it isn't optimised into a FALSE right away. So I'd extend this optimisation, but as a followup PR.

astefan

Left one comment. Otherwise, LGTM.

astefan · 2020-01-08T13:36:38Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-            for (Expression ex : Predicates.splitAnd(and)) {
+            List<Expression> andExps = Predicates.splitAnd(and);
+            // Ranges need to show up before BinaryComparisons in list, to allow the latter be optimized away into a Range, if possible
+            andExps.sort(new Comparator<Expression>() {


Maybe extract this custom Comparator into its own variable inside CombineBinaryComparisons and re-use that, without creating it each time the combine method is called?

Good point, thanks.
I've however replaced with a (non-capturing) lambda, which should then be optimised by the jvm.

Replace anonymous comparator of split AND Expressions with a lambda.

costin · 2020-01-08T16:04:26Z

(1 < a < 3) OR (2 < a < 4) -> (1 < a < 4) that would incorrect though - take 3.5 - it doesn't pass the initial condition but it does the latter.

(1 < a < 2) AND (3 < a < 4) results into 3 < a < 2
should be optimized on the second run. The idea is that each rule modifies on piece, subsequent optimizations are applied by going through the set of rules multiple times until none match.

bpintea · 2020-01-08T17:51:25Z

(1 < a < 3) OR (2 < a < 4) -> (1 < a < 4) that would incorrect though - take 3.5 - it doesn't pass the initial condition but it does the latter.

For conjunction, that wouldn't work, true, but overlapping disjunctions can be optimised with a simple union (3.5 satisfies the 2nd range of the disjunction).

(1 < a < 2) AND (3 < a < 4) results into 3 < a < 2
should be optimized on the second run. The idea is that each rule modifies on piece, subsequent optimizations are applied by going through the set of rules multiple times until none match.

Sure, sure. I was only wondering if this couldn't be achieved in one go.

Anyways, future potential optimisations.

costin · 2020-01-08T18:34:08Z

For conjunction, that wouldn't work, true, but overlapping disjunctions can be optimised with a simple union (3.5 satisfies the 2nd range of the disjunction).

You're right, we should add that too. If you don't plan on working on it in the near future, please make an issue so we don't lose track of it.

Thanks.

* SQL: Optimisation fixes for conjunction merges This commit fixes the following issues around the way comparisions are merged with ranges in conjunctions: * the decision to include the equality of the lower limit is corrected; * the selection of the upper limit is corrected to use the upper bound of the range; * the list of terms in the conjunction is sorted to have the ranges at the bottom; this allows subsequent binary comarisions to find compatible ranges and potentially be merged away. The end guarantee being that the optimisation takes place irrespective of the order of the conjunction terms in the statement. Some comments are also corrected. * adress review observation on anon. comparator Replace anonymous comparator of split AND Expressions with a lambda. (cherry picked from commit 9828cb1)

* SQL: Optimisation fixes for conjunction merges This commit fixes the following issues around the way comparisions are merged with ranges in conjunctions: * the decision to include the equality of the lower limit is corrected; * the selection of the upper limit is corrected to use the upper bound of the range; * the list of terms in the conjunction is sorted to have the ranges at the bottom; this allows subsequent binary comarisions to find compatible ranges and potentially be merged away. The end guarantee being that the optimisation takes place irrespective of the order of the conjunction terms in the statement. Some comments are also corrected. * adress review observation on anon. comparator Replace anonymous comparator of split AND Expressions with a lambda.

bpintea added >bug :Analytics/SQL SQL querying v8.0.0 v7.6.0 v7.5.2 labels Jan 7, 2020

bpintea requested review from costin, astefan and matriv January 7, 2020 14:14

costin approved these changes Jan 7, 2020

View reviewed changes

astefan approved these changes Jan 8, 2020

View reviewed changes

adress review observation on anon. comparator

9587bfe

Replace anonymous comparator of split AND Expressions with a lambda.

bpintea merged commit 9828cb1 into elastic:master Jan 8, 2020

bpintea deleted the fix/conjunctive-comp-in-range branch January 8, 2020 21:36

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

bpintea mentioned this pull request Feb 3, 2020

SQL: Improve optimization of binary comparisons and equal #49637

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Optimisation fixes for conjunction merges #50703

SQL: Optimisation fixes for conjunction merges #50703

bpintea commented Jan 7, 2020 •

edited

Loading

elasticmachine commented Jan 7, 2020

costin left a comment

costin Jan 7, 2020

bpintea Jan 8, 2020

costin Jan 7, 2020

bpintea commented Jan 8, 2020

astefan left a comment

astefan Jan 8, 2020

bpintea Jan 8, 2020

costin commented Jan 8, 2020

bpintea commented Jan 8, 2020

costin commented Jan 8, 2020

SQL: Optimisation fixes for conjunction merges #50703

SQL: Optimisation fixes for conjunction merges #50703

Conversation

bpintea commented Jan 7, 2020 • edited Loading

elasticmachine commented Jan 7, 2020

costin left a comment

Choose a reason for hiding this comment

costin Jan 7, 2020

Choose a reason for hiding this comment

bpintea Jan 8, 2020

Choose a reason for hiding this comment

costin Jan 7, 2020

Choose a reason for hiding this comment

bpintea commented Jan 8, 2020

astefan left a comment

Choose a reason for hiding this comment

astefan Jan 8, 2020

Choose a reason for hiding this comment

bpintea Jan 8, 2020

Choose a reason for hiding this comment

costin commented Jan 8, 2020

bpintea commented Jan 8, 2020

costin commented Jan 8, 2020

bpintea commented Jan 7, 2020 •

edited

Loading