SQL: Reduce number of ranges generated for comparisons (#30267)

* SQL: Reduce number of ranges generated for comparisons

Rewrote optimization rule for combining ranges by improving the
detection of binary comparisons in a tree to better combine
them in a range, regardless of their place inside an expression.
Additionally, improve the comparisons of Numbers of different types
Also, improve reassembly of conjunction/disjunction into balanced
trees.
Do not promote BinaryComparisons to Ranges since it introduces NULL
boundaries and thus a corner-case that needs too much handling
Compare BinaryComparisons directly between themselves and to Ranges

Fix #30017

Author: costin

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java

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

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java

@@ -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 {
 
@@ -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) {
@@ -33,15 +36,51 @@ 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;
+        }
+
+        // clone the list (to modify it)
+        List<Expression> result = new ArrayList<>(exps);
+
+        while (result.size() > 1) {
+            // combine (in place) expressions in pairs
+            // NB: this loop modifies the list (just like an array)
+            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) {
@@ -53,7 +92,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) {
@@ -65,7 +104,7 @@ public static List<Expression> subtract(List<Expression> from, List<Expression>
                 }
             }
         }
-        return diff.isEmpty() ? Collections.emptyList() : diff;
+        return diff.isEmpty() ? emptyList() : diff;
     }
 
 

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java

@@ -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;
@@ -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());
@@ -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();
@@ -122,4 +139,4 @@ public String toString() {
         sb.append(upper);
         return sb.toString();
     }
-}
+}