SQL: Extend the optimisations for equalities #50792

bpintea · 2020-01-09T13:57:50Z

This PR supplements the optimisations of equalities in conjunctions
and disjunctions:

for conjunctions, the existing optimizations with ranges are extended
with not-equalities and inequalities; these lead to a fast resolution,
the conjunction either being evaluate to a FALSE, or the non-equality
conditions being dropped as superfluous;
optimisations for disjunctions are added to be applied against ranges,
inequalities and not-equalities; these lead to disjunction either
becoming TRUE or the equality being dropped, either as superfluous or
merged into a range/inequality.

Adresses #49637

This commit supplements the optimisations of equalities in conjunctions and disjunctions: * for conjunctions, the existing optimizations with ranges are extended with not-equalities and inequalities; these lead to a fast resolution, the conjunction either being evaluate to a FALSE, or the non-equality conditions being dropped as superfluous; * optimisations for disjunctions are added to be applied against ranges, inequalities and not-equalities; these lead to disjunction either becoming TRUE or the equality being dropped, either as superfluous or merged into a range/inequality.

elasticmachine · 2020-01-09T13:57:53Z

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

costin

Left some comments but otherwise LGTM!

costin · 2020-01-10T09:40:00Z

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

-            List<BinaryComparison> equals = new ArrayList<>();
+            // Only equalities, not-equalities and inequalities with a foldable .right are extracted separately;
+            // the others go into the general 'exps'.
+            List<BinaryComparison> frEquals = new ArrayList<>();


what does fr stand for? Can you expand the name to the full word?

costin · 2020-01-10T10:20:24Z

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

+        // a = 2 OR a != 5 -> TRUE; a = 2 OR a = 5 -> nop
+        private Expression propagate(Or or) {
+            List<Expression> exps = new ArrayList<>();
+            List<Equals> frEquals = new ArrayList<>(); // foldable right term Equals


I see that fr comes from foldable right - I would remove the prefix since there's no foldable left and the collections are used for compacting or selecting the expressions.

Sure, removed it. I didn't like it either, tbh, but was trying to avoid the confusion as to why not all references of a certain type are part of the respective container. But with the added comment (and code) it should be clear enough, indeed.

costin · 2020-01-10T10:23:34Z

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

+            List<BinaryComparison> frInequalities = new ArrayList<>(); // foldable right term (=limit) BinaryComparision
+
+            // split expressions by type
+            for (Expression ex : Predicates.splitOr(or)) {


Isn't this handled already by the folding rule + BooleanSimplification?

I believe it's not, no.
(But I'll happily be corrected.)

Or at:

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

Line 838 in 54c1a59

if (bc instanceof Or) {

And at:

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

Line 809 in 54c1a59

if (FALSE.equals(l) || FALSE.equals(r)) {

That is each individual and/or is evaluated if possible

I had a look at that, but the simplification added by the code isn't covered there. Maybe that's what the // TODO: eliminate conjunction/disjunction was hinting to?

(BTW and related to the last suggestions below, the comments (a || b) && (a || c) => a && (b || c) and (a && b) || (a && c) => a || (b & c) will also need fixing (the code does the right thing, though) -- I'll address this too.

costin · 2020-01-10T10:27:03Z

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

+                        updated = true;
+                    } else if (lowerComp != null && upperComp != null) {
+                        if (0 < lowerComp && upperComp < 0) { // a = 2 OR 1 < a < 3
+                            iterEq.remove(); // equality is superfluous


It's worth reviewing the code that does removal through position (for (int i = ...) { ranges.remove(i); } ) since it's incorrect as after a removal it skips the next item as the counter is not decremented.
Moving that to use iterator removal is better (though more verbose).

Right, the ranges index-based looping doesn't do removals in the ranges lists, only updates at current index (for cases like a = 2 OR 2 < a < ? -> 2 <= a < ?); in this case, the removal is operated on the equals list, but that's done through the iterator.
(But let me know please if I still miss something.)

The removal since safe since it is followed by an add the iteration is not affected but inside CombineBinaryComparisons there are some removals that are not compensated for

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

Line 1138 in 54c1a59

bcs.remove(j);

which means the next entry in the bcs list might skipped.
Worth reviewing (and documenting).

But that's a separate issue.

Ah, I see. Sure, I'll address that in a separate issue.

astefan

Nice simplification work.
I have mostly cosmetic related comments, but also one that I believe needs a second look over.

astefan · 2020-01-10T08:20:22Z

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

-            List<BinaryComparison> equals = new ArrayList<>();
+            // Only equalities, not-equalities and inequalities with a foldable .right are extracted separately;
+            // the others go into the general 'exps'.
+            List<BinaryComparison> frEquals = new ArrayList<>();


It took me few seconds to determine what fr stands for. My personal preference is to name these variables as foldRightEq or foldRightEquals.

Right, it wasn't a great solution.. I've gone ahead and adopted Costin's solution to simply remove the prefix. I've considered the foldableRight prefix, but thought that having it would slightly reduce the var distinguishability due to lenght (i.e. foldableRightEqual vs foldableRightNotEqual vs foldableRightInequalities etc.); and fold isn't super clear imo: foldable vs folded vs..?
But no strong feelings about it.

astefan · 2020-01-10T13:23:34Z

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

+                // evaluate all inequalities against the Equal
+                for (Iterator<BinaryComparison> iter = frInequalities.iterator(); iter.hasNext(); ) {
+                    BinaryComparison bc = iter.next();
+                    if (! eq.left().semanticEquals(bc.left())) {


I think we prefer the eq.left().semanticEquals(bc.left()) == false format for better visibility.
Actually, I would have kept the same "style" as the previous conditional statements and not use continue, but the positive outcome of semanticEquals and include the following statements in a bigger if clause.

I think we prefer the eq.left().semanticEquals(bc.left()) == false format for better visibility.

Ah, correct, I think it's even in a guideline somewhere the format with just a variable.

I would have kept the same "style" as the previous conditional statements and not use continue

I find the more convoluted loops easier to read when flattened, but it's only a slight preference, so I've refactored it in this case.

astefan · 2020-01-10T13:25:29Z

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

+                for (Iterator<NotEquals> iter = frNotEquals.iterator(); iter.hasNext(); ) {
+                    NotEquals neq = iter.next();
+                    if (eq.left().semanticEquals(neq.left())) {
+                        Integer comp = BinaryComparison.compare(eq.right().fold(), neq.right().fold());


Instead of eq.right().fold() you could have used eqValue, no?

Right! Thanks.

astefan · 2020-01-10T13:31:02Z

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

+                }
+            }
+
+            return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, frEquals, frNotEquals, frInequalities, ranges)) : and;


return changed == true ? Predicates......

I'd keep this as is, inline with the style of the rest of the class (my change only affected the parameters passed to CollectionUtils.combine).

astefan · 2020-01-10T13:44:31Z

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

+
+                // Equals OR NotEquals
+                for (NotEquals neq : frNotEquals) {
+                    if (eq.left().semanticEquals(neq.left())) { // a = 2 OR a != ? -> TRUE


Why is this evaluated to true right away?

An OR is translated in ES query DSL as a combination of two should statements for a bool query and saying field_x = 2 OR field_x != 4 translates into give me all documents that have field_x = 2 OR field_x != 4 which is in fact a collection of documents with field_x = 2 and documents with field_x != 4. I don't think that statement should be evaluated straight to TRUE, but simplified to field_x != 4.

Evaluating it to TRUE means give me all documents.
Keeping the OR as is means give me the docs with field_x != 4.
These two ^ are not equivalent.

Good spot, thanks!
The TRUE is only valid if the disjunction terms refer the same value. I've fixed it.

Please add a test to double check this is the case.

astefan · 2020-01-10T13:52:25Z

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

+                // Equals OR Range
+                for (int i = 0; i < ranges.size(); i ++) { // might modify list, so use index loop
+                    Range range = ranges.get(i);
+                    if (! eq.left().semanticEquals(range.value())) {


Same as above: == false instead of !.

astefan · 2020-01-10T14:08:20Z

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

+                // Equals OR Inequality
+                for (int i = 0; i < frInequalities.size(); i ++) {
+                    BinaryComparison bc = frInequalities.get(i);
+                    if (! eq.left().semanticEquals(bc.left())) {


astefan · 2020-01-10T14:10:19Z

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

+                            continue;
+                        } else if (comp == 0 && bc instanceof GreaterThan) { // a = 2 OR a > 2 -> a >= 2
+                                frInequalities.set(i, new GreaterThanOrEqual(bc.source(), bc.left(), bc.right()));
+                        } // else (0 < comp) : // a = 3 OR a > 2 -> a > 2


The commented potential condition here - 0 < comp - is not entirely true. A potential else branch can also happen for bc instanceof GreaterThanOrEqual. I don't think it's changing the logic, but just saying.

Right, better be thorough also in comments. Fixed in both affected places.

astefan · 2020-01-10T14:13:57Z

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

+                        iterEq.remove(); // update range with equality instead or simply superfluous
+                        updated = true;
+                    } else if (bc instanceof LessThan || bc instanceof LessThanOrEqual) {
+                        if (0 < comp) { // a = 2 OR a < 1 -> nop


Can you, please, be consistent in the code format? The previous if branch used (comp < 0) and then else if. In this one you are switching places between variable and constant and there is no else.

I generally prefer the "0x axis" spacial ordering for faster understanding - the same reason why ranges are mostly given like a < X < b and less so the other way around -, but I've updated the ordering here.

astefan · 2020-01-10T14:14:37Z

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

            }

-            return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, equals, ranges)) : and;
+            return updated ? Predicates.combineOr(CollectionUtils.combine(exps, frEquals, frNotEquals, frInequalities, ranges)) : or;


updated == true ? Predicates.....

* Fix the bug around wrongly optimizing 'a=2 OR a!=?', which only yields TRUE for same values in equality and inequality. * Var renamings, code style adjustments, comments corrections.

costin · 2020-01-10T20:06:59Z

As a separate improvements, we should move away from returning Literal.TRUE/FALSE since these constants don't preserve the Source, which typically is not needed but why drop it.
Instead the Source of the folded expression should be kept around, e.g.:

return FALSE -> return Literal.of(bc, Boolean.FALSE);

astefan

LGTM

- fix a few code comments; - extend the Equals OR NotEquals optimitsation (a=2 OR a!=5 -> a!=5); - extend the Equals OR Range optimisation on limits equality (a=2 OR 2<=a<5 -> 2<=a<5); - in case an equality is being removed in a conjunction, the rest of possible optimisations to test is now skipped.

- s/rmEqual/removeEquals

…als-optimisations

costin · 2020-01-14T10:34:24Z

As a separate improvements, we should move away from returning Literal.TRUE/FALSE since these constants don't preserve the Source, which typically is not needed but why drop it.
Instead the Source of the folded expression should be kept around, e.g.:
return FALSE -> return Literal.of(bc, Boolean.FALSE);

I've addressed that in #50962

* Extend the optimizations for equalities This commit supplements the optimisations of equalities in conjunctions and disjunctions: * for conjunctions, the existing optimizations with ranges are extended with not-equalities and inequalities; these lead to a fast resolution, the conjunction either being evaluate to a FALSE, or the non-equality conditions being dropped as superfluous; * optimisations for disjunctions are added to be applied against ranges, inequalities and not-equalities; these lead to disjunction either becoming TRUE or the equality being dropped, either as superfluous or merged into a range/inequality. * Adress review notes * Fix the bug around wrongly optimizing 'a=2 OR a!=?', which only yields TRUE for same values in equality and inequality. * Var renamings, code style adjustments, comments corrections. * Address further review comments. Extend optim. - fix a few code comments; - extend the Equals OR NotEquals optimitsation (a=2 OR a!=5 -> a!=5); - extend the Equals OR Range optimisation on limits equality (a=2 OR 2<=a<5 -> 2<=a<5); - in case an equality is being removed in a conjunction, the rest of possible optimisations to test is now skipped. * rename one var for better legiblity - s/rmEqual/removeEquals (cherry picked from commit 62e7c6a)

* Extend the optimizations for equalities This commit supplements the optimisations of equalities in conjunctions and disjunctions: * for conjunctions, the existing optimizations with ranges are extended with not-equalities and inequalities; these lead to a fast resolution, the conjunction either being evaluate to a FALSE, or the non-equality conditions being dropped as superfluous; * optimisations for disjunctions are added to be applied against ranges, inequalities and not-equalities; these lead to disjunction either becoming TRUE or the equality being dropped, either as superfluous or merged into a range/inequality. * Adress review notes * Fix the bug around wrongly optimizing 'a=2 OR a!=?', which only yields TRUE for same values in equality and inequality. * Var renamings, code style adjustments, comments corrections. * Address further review comments. Extend optim. - fix a few code comments; - extend the Equals OR NotEquals optimitsation (a=2 OR a!=5 -> a!=5); - extend the Equals OR Range optimisation on limits equality (a=2 OR 2<=a<5 -> 2<=a<5); - in case an equality is being removed in a conjunction, the rest of possible optimisations to test is now skipped. * rename one var for better legiblity - s/rmEqual/removeEquals

bpintea added >enhancement :Analytics/SQL SQL querying v8.0.0 v7.6.0 labels Jan 9, 2020

bpintea requested review from costin, astefan and matriv January 9, 2020 13:57

costin approved these changes Jan 10, 2020

View reviewed changes

astefan requested changes Jan 10, 2020

View reviewed changes

Adress review notes

3eee6ca

* Fix the bug around wrongly optimizing 'a=2 OR a!=?', which only yields TRUE for same values in equality and inequality. * Var renamings, code style adjustments, comments corrections.

astefan approved these changes Jan 13, 2020

View reviewed changes

bpintea requested a review from astefan January 13, 2020 15:43

bpintea added 2 commits January 13, 2020 17:32

rename one var for better legiblity

cab0248

- s/rmEqual/removeEquals

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

7755d1d

…als-optimisations

bpintea merged commit 62e7c6a into elastic:master Jan 14, 2020

bpintea deleted the enh/propagate-equals-optimisations branch January 14, 2020 16:59

bpintea added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

bpintea mentioned this pull request Feb 3, 2020

SQL: Improve optimization of binary comparisons and equal #49637

Closed

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

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

SQL: Extend the optimisations for equalities #50792

SQL: Extend the optimisations for equalities #50792

Conversation

bpintea commented Jan 9, 2020

elasticmachine commented Jan 9, 2020

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

Choose a reason for hiding this comment

costin Jan 10, 2020 • edited Loading

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

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

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

costin commented Jan 10, 2020

astefan left a comment

Choose a reason for hiding this comment

costin commented Jan 14, 2020

costin Jan 10, 2020 •

edited

Loading