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.

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

SQL: Extend the optimisations for equalities #50792

Merged
merged 5 commits into from
Jan 14, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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 @@ -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,169 @@ 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 != 5 -> TRUE; a = 2 OR a = 5 -> nop
Copy link
Contributor

@astefan astefan Jan 10, 2020

Choose a reason for hiding this comment

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

a = 2 OR a != 5 -> TRUE I think it's not right. Please, see above more details about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks. Will update along the a = 2 OR a != 5 -> a != 5 change.

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:


And at:

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();

// Equals OR NotEquals
for (NotEquals neq : notEquals) {
if (eq.left().semanticEquals(neq.left())) { // a = 2 OR a != 2 -> TRUE
Integer comp = BinaryComparison.compare(eqValue, neq.right().fold());
if (comp != null && comp == 0) {
return TRUE;
}
}
}

// 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()) == false) {
continue;
}

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 && !range.includeLower()) { // a = 2 OR 2 < a < ? -> 2 <= a < ?
iterEq.remove(); // update range with lower equality instead
ranges.set(i, new Range(range.source(), range.value(), range.lower(), true, range.upper(), range.includeUpper()));
updated = true;
} else if (upperComp != null && upperComp == 0 && !range.includeUpper()) { // a = 2 OR ? < a < 2 -> ? < a <= 2
iterEq.remove(); // update range with upper equality instead
ranges.set(i, new Range(range.source(), range.value(), range.lower(), range.includeLower(), range.upper(), true));
updated = true;
} else if (lowerComp != null && upperComp != null) {
if (0 < lowerComp && upperComp < 0) { // a = 2 OR 1 < a < 3
iterEq.remove(); // equality is superfluous
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Member

@costin costin Jan 10, 2020

Choose a reason for hiding this comment

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

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


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

But that's a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

updated = true;
}
}
}

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

Integer comp = BinaryComparison.compare(eqValue, bc.right().fold());
if (comp == null) {
continue;
}
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
iterEq.remove(); // update range with equality instead or simply superfluous
updated = true;
} 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
iterEq.remove(); // update range with equality instead or simply superfluous
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