Skip to content

SQL: Reduce number of ranges generated for comparisons #30267

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

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 1 commit
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
31 changes: 27 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 @@ -34,10 +33,34 @@ public DataType dataType() {
}

@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);
}

try {
if (l instanceof Comparable && r instanceof Comparable) {
return Integer.valueOf(((Comparable) l).compareTo(r));
}
} catch (ClassCastException cce) {
// types are not compatible
// return null
Copy link
Member

@nik9000 nik9000 Apr 30, 2018

Choose a reason for hiding this comment

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

What are the side effects on not catching this and letting it bubble out? Or even wrapping it? I think it'd be nice to have a big comment here about why returning null is ok.

Also, can you move the try into the if statement so it looks smaller? That'd just make me a bit more comfortable. If returning null is right here then I think you should return it from the catch rather than fall out. That feels a little safer even if it isn't.

Copy link
Member Author

@costin costin Apr 30, 2018

Choose a reason for hiding this comment

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

CCE is thrown when the types are not compatible; I've expanded the comment and returns the null directly from catch.

}
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()));
}
}
40 changes: 33 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,38 @@ 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' - while a bit longer this method creates a balanced tree as oppose to a plain
Copy link
Member

@nik9000 nik9000 Apr 30, 2018

Choose a reason for hiding this comment

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

Maybe

Builds a "pyramid" out of all clauses by combining them pairwise. So
{@code combine(List(a, b, c, d, e), AND)` becomes
<pre><code>
       AND
   |---/ \---|
  AND        AND
 /  \        / \
e   AND     b   c
    / \    
   a   b   
</pre></code>

The picture would make me feel better.

Copy link
Member Author

@costin costin Apr 30, 2018

Choose a reason for hiding this comment

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

Done.

* recursive approach that 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

@nik9000 nik9000 Apr 30, 2018

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

@nik9000 nik9000 Apr 30, 2018

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

@costin costin May 2, 2018

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

@nik9000 nik9000 May 2, 2018

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 +79,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 +91,7 @@ public static List<Expression> subtract(List<Expression> from, List<Expression>
}
}
}
return diff.isEmpty() ? Collections.emptyList() : diff;
return diff.isEmpty() ? emptyList() : diff;
}


Expand Down
18 changes: 16 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 (excludingBoundaries() || value.foldable());
Copy link
Member

@nik9000 nik9000 Apr 30, 2018

Choose a reason for hiding this comment

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

Can you remove the extra ( and )? I don't think they help here.

Copy link
Member Author

@costin costin May 2, 2018

Choose a reason for hiding this comment

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

Done

}

return false;
}

@Override
public Object fold() {
if (excludingBoundaries()) {
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,13 @@ public Object fold() {
return lowerComparsion && upperComparsion;
}

private boolean excludingBoundaries() {
Copy link
Member

@nik9000 nik9000 Apr 30, 2018

Choose a reason for hiding this comment

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

Can you move the explanation to javadoc? I do like that you catch this case. I might rename this method. I'd call it isDegenerate but I think that isn't a great name either. matchesNothing?

Copy link
Member Author

@costin costin Apr 30, 2018

Choose a reason for hiding this comment

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

Done.

// if the boundaries exclude each other, the value doesn't need to be evaluated
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