Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SQL: Extend the optimisations for equalities #50792

Merged
merged 5 commits into from
Jan 14, 2020
Merged

SQL: Extend the optimisations for equalities #50792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59bb258
Extend the optimizations for equalities
bpintea Jan 9, 2020
3eee6ca
Adress review notes
bpintea Jan 10, 2020
55da035
Address further review comments. Extend optim.
bpintea Jan 13, 2020
cab0248
rename one var for better legiblity
bpintea Jan 13, 2020
7755d1d
Merge remote-tracking branch 'upstream/master' into enh/propagate-equ…
bpintea Jan 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 221 additions & 22 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
Expand Down Expand Up @@ -814,7 +815,7 @@ private Expression simplifyAndOr(BinaryPredicate<?, ?, ?, ?> bc) {
}

//
// common factor extraction -> (a || b) && (a || c) => a && (b || c)
// common factor extraction -> (a || b) && (a || c) => a || (b && c)
//
List<Expression> leftSplit = splitOr(l);
List<Expression> rightSplit = splitOr(r);
Expand Down Expand Up @@ -852,7 +853,7 @@ private Expression simplifyAndOr(BinaryPredicate<?, ?, ?, ?> bc) {
}

//
// common factor extraction -> (a && b) || (a && c) => a || (b & c)
// common factor extraction -> (a && b) || (a && c) => a && (b || c)
//
List<Expression> leftSplit = splitAnd(l);
List<Expression> rightSplit = splitAnd(r);
Expand Down Expand Up @@ -958,9 +959,11 @@ private Expression literalToTheRight(BinaryOperator<?, ?, ?, ?> be) {
}

/**
* Propagate Equals to eliminate conjuncted Ranges.
* When encountering a different Equals or non-containing {@link Range}, the conjunction becomes false.
* When encountering a containing {@link Range}, the range gets eliminated by the equality.
* Propagate Equals to eliminate conjuncted Ranges or BinaryComparisons.
* When encountering a different Equals, non-containing {@link Range} or {@link BinaryComparison}, the conjunction becomes false.
* When encountering a containing {@link Range}, {@link BinaryComparison} or {@link NotEquals}, these get eliminated by the equality.
*
* Since this rule can eliminate Ranges and BinaryComparisons, it should be applied before {@link CombineBinaryComparisons}.
*
* This rule doesn't perform any promotion of {@link BinaryComparison}s, that is handled by
* {@link CombineBinaryComparisons} on purpose as the resulting Range might be foldable
Expand All @@ -976,14 +979,20 @@ static class PropagateEquals extends OptimizerExpressionRule {
protected Expression rule(Expression e) {
if (e instanceof And) {
return propagate((And) e);
} else if (e instanceof Or) {
return propagate((Or) e);
}
return e;
}

// combine conjunction
private Expression propagate(And and) {
List<Range> ranges = new ArrayList<>();
// Only equalities, not-equalities and inequalities with a foldable .right are extracted separately;
// the others go into the general 'exps'.
List<BinaryComparison> equals = new ArrayList<>();
List<NotEquals> notEquals = new ArrayList<>();
List<BinaryComparison> inequalities = new ArrayList<>();
List<Expression> exps = new ArrayList<>();

boolean changed = false;
Expand All @@ -996,35 +1005,42 @@ private Expression propagate(And and) {
// equals on different values evaluate to FALSE
if (otherEq.right().foldable()) {
for (BinaryComparison eq : equals) {
// cannot evaluate equals so skip it
if (!eq.right().foldable()) {
continue;
}
if (otherEq.left().semanticEquals(eq.left())) {
if (eq.right().foldable() && otherEq.right().foldable()) {
Integer comp = BinaryComparison.compare(eq.right().fold(), otherEq.right().fold());
if (comp != null) {
// var cannot be equal to two different values at the same time
if (comp != 0) {
return FALSE;
}
Integer comp = BinaryComparison.compare(eq.right().fold(), otherEq.right().fold());
if (comp != null) {
// var cannot be equal to two different values at the same time
if (comp != 0) {
return FALSE;
}
}
}
}
equals.add(otherEq);
} else {
exps.add(otherEq);
}
} else if (ex instanceof GreaterThan || ex instanceof GreaterThanOrEqual ||
ex instanceof LessThan || ex instanceof LessThanOrEqual) {
BinaryComparison bc = (BinaryComparison) ex;
if (bc.right().foldable()) {
inequalities.add(bc);
} else {
exps.add(ex);
}
} else if (ex instanceof NotEquals) {
NotEquals otherNotEq = (NotEquals) ex;
if (otherNotEq.right().foldable()) {
notEquals.add(otherNotEq);
} else {
exps.add(ex);
}
equals.add(otherEq);
} else {
exps.add(ex);
}
}

// check
for (BinaryComparison eq : equals) {
// cannot evaluate equals so skip it
if (!eq.right().foldable()) {
continue;
}
Object eqValue = eq.right().fold();

for (int i = 0; i < ranges.size(); i++) {
Expand Down Expand Up @@ -1060,9 +1076,192 @@ private Expression propagate(And and) {
changed = true;
}
}

// evaluate all NotEquals against the Equal
for (Iterator<NotEquals> iter = notEquals.iterator(); iter.hasNext(); ) {
NotEquals neq = iter.next();
if (eq.left().semanticEquals(neq.left())) {
Integer comp = BinaryComparison.compare(eqValue, neq.right().fold());
if (comp != null) {
if (comp == 0) {
return FALSE; // clashing and conflicting: a = 1 AND a != 1
} else {
iter.remove(); // clashing and redundant: a = 1 AND a != 2
changed = true;
}
}
}
}

// evaluate all inequalities against the Equal
for (Iterator<BinaryComparison> iter = inequalities.iterator(); iter.hasNext(); ) {
BinaryComparison bc = iter.next();
if (eq.left().semanticEquals(bc.left())) {
Integer compare = BinaryComparison.compare(eqValue, bc.right().fold());
if (compare != null) {
if (bc instanceof LessThan || bc instanceof LessThanOrEqual) { // a = 2 AND a </<= ?
if ((compare == 0 && bc instanceof LessThan) || // a = 2 AND a < 2
0 < compare) { // a = 2 AND a </<= 1
return FALSE;
}
} else if (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual) { // a = 2 AND a >/>= ?
if ((compare == 0 && bc instanceof GreaterThan) || // a = 2 AND a > 2
compare < 0) { // a = 2 AND a >/>= 3
return FALSE;
}
}

iter.remove();
changed = true;
}
}
}
}

return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, equals, notEquals, inequalities, ranges)) : and;
}

// combine disjunction:
// a = 2 OR a > 3 -> nop; a = 2 OR a > 1 -> a > 1
// a = 2 OR a < 3 -> a < 3; a = 2 OR a < 1 -> nop
// a = 2 OR 3 < a < 5 -> nop; a = 2 OR 1 < a < 3 -> 1 < a < 3; a = 2 OR 0 < a < 1 -> nop
// a = 2 OR a != 2 -> TRUE; a = 2 OR a = 5 -> nop; a = 2 OR a != 5 -> a != 5
private Expression propagate(Or or) {
List<Expression> exps = new ArrayList<>();
List<Equals> equals = new ArrayList<>(); // foldable right term Equals
List<NotEquals> notEquals = new ArrayList<>(); // foldable right term NotEquals
List<Range> ranges = new ArrayList<>();
List<BinaryComparison> inequalities = new ArrayList<>(); // foldable right term (=limit) BinaryComparision

// split expressions by type
for (Expression ex : Predicates.splitOr(or)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (ex instanceof Equals) {
Equals eq = (Equals) ex;
if (eq.right().foldable()) {
equals.add(eq);
} else {
exps.add(ex);
}
} else if (ex instanceof NotEquals) {
NotEquals neq = (NotEquals) ex;
if (neq.right().foldable()) {
notEquals.add(neq);
} else {
exps.add(ex);
}
} else if (ex instanceof Range) {
ranges.add((Range) ex);
} else if (ex instanceof BinaryComparison) {
BinaryComparison bc = (BinaryComparison) ex;
if (bc.right().foldable()) {
inequalities.add(bc);
} else {
exps.add(ex);
}
} else {
exps.add(ex);
}
}

boolean updated = false; // has the expression been modified?

// evaluate the impact of each Equal over the different types of Expressions
for (Iterator<Equals> iterEq = equals.iterator(); iterEq.hasNext(); ) {
Equals eq = iterEq.next();
Object eqValue = eq.right().fold();
boolean removeEquals = false;

// Equals OR NotEquals
for (NotEquals neq : notEquals) {
if (eq.left().semanticEquals(neq.left())) { // a = 2 OR a != ? -> ...
Integer comp = BinaryComparison.compare(eqValue, neq.right().fold());
if (comp != null) {
if (comp == 0) { // a = 2 OR a != 2 -> TRUE
return TRUE;
} else { // a = 2 OR a != 5 -> a != 5
removeEquals = true;
break;
}
}
}
}
if (removeEquals) {
iterEq.remove();
updated = true;
continue;
}

// 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())) {
Integer lowerComp = range.lower().foldable() ? BinaryComparison.compare(eqValue, range.lower().fold()) : null;
Integer upperComp = range.upper().foldable() ? BinaryComparison.compare(eqValue, range.upper().fold()) : null;

if (lowerComp != null && lowerComp == 0) {
if (!range.includeLower()) { // a = 2 OR 2 < a < ? -> 2 <= a < ?
ranges.set(i, new Range(range.source(), range.value(), range.lower(), true,
range.upper(), range.includeUpper()));
} // else : a = 2 OR 2 <= a < ? -> 2 <= a < ?
removeEquals = true; // update range with lower equality instead or simply superfluous
break;
} else if (upperComp != null && upperComp == 0) {
if (!range.includeUpper()) { // a = 2 OR ? < a < 2 -> ? < a <= 2
ranges.set(i, new Range(range.source(), range.value(), range.lower(), range.includeLower(),
range.upper(), true));
} // else : a = 2 OR ? < a <= 2 -> ? < a <= 2
removeEquals = true; // update range with upper equality instead
break;
} else if (lowerComp != null && upperComp != null) {
if (0 < lowerComp && upperComp < 0) { // a = 2 OR 1 < a < 3
removeEquals = true; // equality is superfluous
break;
}
}
}
}
if (removeEquals) {
iterEq.remove();
updated = true;
continue;
}

// Equals OR Inequality
for (int i = 0; i < inequalities.size(); i ++) {
BinaryComparison bc = inequalities.get(i);
if (eq.left().semanticEquals(bc.left())) {
Integer comp = BinaryComparison.compare(eqValue, bc.right().fold());
if (comp != null) {
if (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual) {
if (comp < 0) { // a = 1 OR a > 2 -> nop
continue;
} else if (comp == 0 && bc instanceof GreaterThan) { // a = 2 OR a > 2 -> a >= 2
inequalities.set(i, new GreaterThanOrEqual(bc.source(), bc.left(), bc.right()));
} // else (0 < comp || bc instanceof GreaterThanOrEqual) :
// a = 3 OR a > 2 -> a > 2; a = 2 OR a => 2 -> a => 2

removeEquals = true; // update range with equality instead or simply superfluous
break;
} else if (bc instanceof LessThan || bc instanceof LessThanOrEqual) {
if (comp > 0) { // a = 2 OR a < 1 -> nop
continue;
}
if (comp == 0 && bc instanceof LessThan) { // a = 2 OR a < 2 -> a <= 2
inequalities.set(i, new LessThanOrEqual(bc.source(), bc.left(), bc.right()));
} // else (comp < 0 || bc instanceof LessThanOrEqual) : a = 2 OR a < 3 -> a < 3; a = 2 OR a <= 2 -> a <= 2
removeEquals = true; // update range with equality instead or simply superfluous
break;
}
}
}
}
if (removeEquals) {
iterEq.remove();
updated = true;
}
}

return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, equals, ranges)) : and;
return updated ? Predicates.combineOr(CollectionUtils.combine(exps, equals, notEquals, inequalities, ranges)) : or;
}
}

Expand Down
Loading