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: Reduce number of ranges generated for comparisons #30267

Merged
merged 6 commits into from
May 2, 2018
Merged

SQL: Reduce number of ranges generated for comparisons #30267

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d507741
SQL: Reduce number of ranges generated for comparisons
costin Apr 26, 2018
84872b4
Address feedback
costin Apr 30, 2018
f0d7d4c
Improve rule for optimizing binary comparison
costin May 1, 2018
a3aca69
Merge remote-tracking branch 'remotes/upstream/master' into better-bi…
costin May 2, 2018
25aed21
Fix tests
costin May 2, 2018
a2fb0d2
Add extra comments
costin May 2, 2018
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
38 changes: 34 additions & 4 deletions .../sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.tree.Location;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DataTypes;

// marker class to indicate operations that rely on values
public abstract class BinaryComparison extends BinaryOperator {
Expand All @@ -33,11 +32,42 @@ public DataType dataType() {
return DataType.BOOLEAN;
}

/**
* Compares two expression arguments (typically Numbers), if possible.
* Otherwise returns null (the arguments are not comparable or at least
* one of them is null).
*/
@SuppressWarnings({ "rawtypes", "unchecked" })
static Integer compare(Object left, Object right) {
if (left instanceof Comparable && right instanceof Comparable) {
return Integer.valueOf(((Comparable) left).compareTo(right));
public static Integer compare(Object l, Object r) {
// typical number comparison
if (l instanceof Number && r instanceof Number) {
return compare((Number) l, (Number) r);
}

if (l instanceof Comparable && r instanceof Comparable) {
try {
return Integer.valueOf(((Comparable) l).compareTo(r));
} catch (ClassCastException cce) {
// when types are not compatible, cce is thrown
// fall back to null
return null;
}
}

return null;
}

static Integer compare(Number l, Number r) {
if (l instanceof Double || r instanceof Double) {
return Double.compare(l.doubleValue(), r.doubleValue());
}
if (l instanceof Float || r instanceof Float) {
return Float.compare(l.floatValue(), r.floatValue());
}
if (l instanceof Long || r instanceof Long) {
return Long.compare(l.longValue(), r.longValue());
}

return Integer.valueOf(Integer.compare(l.intValue(), r.intValue()));
}
}
51 changes: 44 additions & 7 deletions ...plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.function.BiFunction;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;

public abstract class Predicates {

Expand All @@ -22,7 +25,7 @@ public static List<Expression> splitAnd(Expression exp) {
list.addAll(splitAnd(and.right()));
return list;
}
return Collections.singletonList(exp);
return singletonList(exp);
}

public static List<Expression> splitOr(Expression exp) {
Expand All @@ -33,15 +36,49 @@ public static List<Expression> splitOr(Expression exp) {
list.addAll(splitOr(or.right()));
return list;
}
return Collections.singletonList(exp);
return singletonList(exp);
}

public static Expression combineOr(List<Expression> exps) {
return exps.stream().reduce((l, r) -> new Or(l.location(), l, r)).orElse(null);
return combine(exps, (l, r) -> new Or(l.location(), l, r));
}

public static Expression combineAnd(List<Expression> exps) {
return exps.stream().reduce((l, r) -> new And(l.location(), l, r)).orElse(null);
return combine(exps, (l, r) -> new And(l.location(), l, r));
}

/**
* Build a binary 'pyramid' from the given list:
* <pre>
* AND
* / \
* AND AND
* / \ / \
* A B C D
* </pre>
*
* using the given combiner.
*
* While a bit longer, this method creates a balanced tree as oppose to a plain
* recursive approach which creates an unbalanced one (either to the left or right).
*/
private static Expression combine(List<Expression> exps, BiFunction<Expression, Expression, Expression> combiner) {
if (exps.isEmpty()) {
return null;
}

List<Expression> result = new ArrayList<>(exps);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be Queue<Expression> work = new ArrayDeque<>(exps); and then you do work.addLast(combiner.apply(work.removeFirst(), work.removeFirst())); The for loop and remove together scare me.

Copy link
Member

Choose a reason for hiding this comment

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

I talked with @costin earlier about this - he wants to keep the order the same and my proposal doesn't.

What about this?

while (result.size() > 1) {
  ListIterator<Expression> itr = result.iterator();
  while (itr.hasNext()) {
    itr.add(combiner.apply(itr.remove(), itr.remove()));
  }
}

Your version works but for (int i = 0; i < result.size() - 1; i++) { make me think it'll be a normal loop and then you remove and add and I'm confused. Using the ListIterator forces the reader to think.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you'd like we can follow this up in a different PR.
The approach above doesn't work since remove can only be called once per next/previous.

I've added a comment to the loop explaining that it updates the list (and thus why it uses the temporary variable).
Considering the loops are fairly contained I think that conveys the message without the gymnastics of iterator.

P.S. The list is created just above as a copy for this specific reason.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving it, yeah. I did want a prettier one but if this is what we can do, it'll do.


while (result.size() > 1) {
// combine (in place) expressions in pairs
for (int i = 0; i < result.size() - 1; i++) {
Expression l = result.remove(i);
Expression r = result.remove(i);
result.add(i, combiner.apply(l, r));
}
}

return result.get(0);
}

public static List<Expression> inCommon(List<Expression> l, List<Expression> r) {
Expand All @@ -53,7 +90,7 @@ public static List<Expression> inCommon(List<Expression> l, List<Expression> r)
}
}
}
return common.isEmpty() ? Collections.emptyList() : common;
return common.isEmpty() ? emptyList() : common;
}

public static List<Expression> subtract(List<Expression> from, List<Expression> r) {
Expand All @@ -65,7 +102,7 @@ public static List<Expression> subtract(List<Expression> from, List<Expression>
}
}
}
return diff.isEmpty() ? Collections.emptyList() : diff;
return diff.isEmpty() ? emptyList() : diff;
}


Expand Down
21 changes: 19 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.elasticsearch.xpack.sql.tree.Location;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DataTypes;

import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -66,11 +65,19 @@ public boolean includeUpper() {

@Override
public boolean foldable() {
return value.foldable() && lower.foldable() && upper.foldable();
if (lower.foldable() && upper.foldable()) {
return areBoundariesInvalid() || value.foldable();
}

return false;
}

@Override
public Object fold() {
if (areBoundariesInvalid()) {
return Boolean.FALSE;
}

Object val = value.fold();
Integer lowerCompare = BinaryComparison.compare(lower.fold(), val);
Integer upperCompare = BinaryComparison.compare(val, upper().fold());
Expand All @@ -79,6 +86,16 @@ public Object fold() {
return lowerComparsion && upperComparsion;
}

/**
* Check whether the boundaries are invalid ( upper < lower) or not.
* If they do, the value does not have to be evaluate.
*/
private boolean areBoundariesInvalid() {
Integer compare = BinaryComparison.compare(lower.fold(), upper.fold());
// upper < lower OR upper == lower and the range doesn't contain any equals
return compare != null && (compare > 0 || (compare == 0 && (!includeLower || !includeUpper)));
}

@Override
public boolean nullable() {
return value.nullable() && lower.nullable() && upper.nullable();
Expand Down
Loading