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

SQL: Optimisation fixes for conjunction merges #50703

Merged
merged 2 commits into from
Jan 8, 2020
Merged

SQL: Optimisation fixes for conjunction merges #50703

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8a73cd
SQL: Optimisation fixes for conjunction merges
bpintea Jan 7, 2020
9587bfe
adress review observation on anon. comparator
bpintea Jan 8, 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
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
* In a SQL statement, an Expression is whatever a user specifies inside an
* action, so for instance:
*
* {@code SELECT a, b, MAX(c, d) FROM i}
* {@code SELECT a, b, ABS(c) FROM i}
*
* a, b, ABS(c), and i are all Expressions, with ABS(c) being a Function
* (which is a type of expression) with a single child, c.
Expand Down
33 changes: 24 additions & 9 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 @@ -92,6 +92,7 @@
import java.util.Map.Entry;
import java.util.Set;
import java.util.function.Consumer;
import java.util.Comparator;

import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.sql.expression.Expressions.equalsAsAttribute;
Expand Down Expand Up @@ -397,7 +398,7 @@ protected LogicalPlan rule(OrderBy ob) {
return ob;
}
}

static class PruneOrderByForImplicitGrouping extends OptimizerRule<OrderBy> {

@Override
Expand Down Expand Up @@ -1090,7 +1091,21 @@ private Expression combine(And and) {

boolean changed = false;

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>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Override
public int compare(Expression o1, Expression o2) {
if (o1 instanceof Range && o2 instanceof Range) {
return 0; // keep ranges' order
} else if (o1 instanceof Range || o2 instanceof Range) {
return o2 instanceof Range ? 1 : -1;
} else {
return 0; // keep non-ranges' order
}
}
});
for (Expression ex : andExps) {
if (ex instanceof Range) {
Range r = (Range) ex;
if (findExistingRange(r, ranges, true)) {
Expand Down Expand Up @@ -1215,9 +1230,9 @@ private static boolean findExistingRange(Range main, List<Range> ranges, boolean
lowerEq = comp == 0 && main.includeLower() == other.includeLower();
// AND
if (conjunctive) {
// (2 < a < 3) AND (1 < a < 3) -> (1 < a < 3)
// (2 < a < 3) AND (1 < a < 3) -> (2 < a < 3)
lower = comp > 0 ||
// (2 < a < 3) AND (2 < a <= 3) -> (2 < a < 3)
// (2 < a < 3) AND (2 <= a < 3) -> (2 < a < 3)
(comp == 0 && !main.includeLower() && other.includeLower());
}
// OR
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

That's a big miss - were there no tests for 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.

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

other.upper(), other.includeUpper()));
}

Expand All @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - a whole optimization being skipped...

Integer comp = BinaryComparison.compare(value, other.upper().fold());
if (comp != null) {
// a < 2 AND (1 < a <= 2) -> 1 < a < 2
boolean upperEq = comp == 0 && other.includeUpper() && main instanceof LessThan;
// a < 2 AND (1 < a < 3) -> 1 < a < 2
boolean upper = comp > 0 || upperEq;
boolean upper = comp < 0 || upperEq;

if (upper) {
ranges.remove(i);
ranges.add(i, new Range(other.source(), other.value(),
other.lower(), other.includeLower(),
main.right(), upperEq ? true : other.includeUpper()));
main.right(), upperEq ? false : main instanceof LessThanOrEqual));
}

// found a match
Expand Down
51 changes: 51 additions & 0 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,57 @@ public void testCombineBinaryComparisonsInclude() {
assertEquals(FIVE, r.right());
}

// 2 < a AND (2 <= a < 3) -> 2 < a < 3
public void testCombineBinaryComparisonsAndRangeLower() {
FieldAttribute fa = getFieldAttribute();

GreaterThan gt = new GreaterThan(EMPTY, fa, TWO);
Range range = new Range(EMPTY, fa, TWO, true, THREE, false);

CombineBinaryComparisons rule = new CombineBinaryComparisons();
Expression exp = rule.rule(new And(EMPTY, gt, range));
assertEquals(Range.class, exp.getClass());
Range r = (Range)exp;
assertEquals(TWO, r.lower());
assertFalse(r.includeLower());
assertEquals(THREE, r.upper());
assertFalse(r.includeUpper());
}

// a < 4 AND (1 < a < 3) -> 1 < a < 3
public void testCombineBinaryComparisonsAndRangeUpper() {
FieldAttribute fa = getFieldAttribute();

LessThan lt = new LessThan(EMPTY, fa, FOUR);
Range range = new Range(EMPTY, fa, ONE, false, THREE, false);

CombineBinaryComparisons rule = new CombineBinaryComparisons();
Expression exp = rule.rule(new And(EMPTY, range, lt));
assertEquals(Range.class, exp.getClass());
Range r = (Range)exp;
assertEquals(ONE, r.lower());
assertFalse(r.includeLower());
assertEquals(THREE, r.upper());
assertFalse(r.includeUpper());
}

// a <= 2 AND (1 < a < 3) -> 1 < a <= 2
public void testCombineBinaryComparisonsAndRangeUpperEqual() {
FieldAttribute fa = getFieldAttribute();

LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, TWO);
Range range = new Range(EMPTY, fa, ONE, false, THREE, false);

CombineBinaryComparisons rule = new CombineBinaryComparisons();
Expression exp = rule.rule(new And(EMPTY, lte, range));
assertEquals(Range.class, exp.getClass());
Range r = (Range)exp;
assertEquals(ONE, r.lower());
assertFalse(r.includeLower());
assertEquals(TWO, r.upper());
assertTrue(r.includeUpper());
}

// 3 <= a AND 4 < a AND a <= 7 AND a < 6 -> 4 < a < 6
public void testCombineMultipleBinaryComparisons() {
FieldAttribute fa = getFieldAttribute();
Expand Down