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: 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 all commits
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
29 changes: 20 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 @@ -397,7 +397,7 @@ protected LogicalPlan rule(OrderBy ob) {
return ob;
}
}

static class PruneOrderByForImplicitGrouping extends OptimizerRule<OrderBy> {

@Override
Expand Down Expand Up @@ -1090,7 +1090,18 @@ 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((o1, 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 +1226,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 +1327,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 +1336,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