Browse Source

SQL: Handle null literal for AND and OR in `WHERE` (#35236)

Change `nullable()` logic of AND and OR to false since in the Optimizer
we cannot fold to null as we might be handling and expression in the
SELECT clause.

Introduce folding of null for AND and OR expressions in PruneFilter()
since we now know that we are in HAVING or WHERE clause and we
can fold `null` to `false`

Fixes: #35088

Co-authored-by: Costin Leau <costin.leau@gmail.com>
Marios Trivyzas 7 years ago
parent
commit
c9a84fe361

+ 2 - 1
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/BinaryLogic.java

@@ -36,6 +36,7 @@ public abstract class BinaryLogic extends BinaryOperator<Boolean, Boolean, Boole
 
     @Override
     public boolean nullable() {
-        return left().nullable() && right().nullable();
+        // Cannot fold null due to 3vl, constant folding will do any possible folding.
+        return false;
     }
 }

+ 37 - 11
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -685,17 +685,46 @@ public class Optimizer extends RuleExecutor<LogicalPlan> {
 
         @Override
         protected LogicalPlan rule(Filter filter) {
-            if (filter.condition() instanceof Literal) {
-                if (TRUE.equals(filter.condition())) {
+            Expression condition = filter.condition().transformUp(PruneFilters::foldBinaryLogic);
+
+            if (condition instanceof Literal) {
+                if (TRUE.equals(condition)) {
                     return filter.child();
                 }
-                if (FALSE.equals(filter.condition()) || Expressions.isNull(filter.condition())) {
+                if (FALSE.equals(condition) || Expressions.isNull(condition)) {
                     return new LocalRelation(filter.location(), new EmptyExecutable(filter.output()));
                 }
             }
 
+            if (!condition.equals(filter.condition())) {
+                return new Filter(filter.location(), filter.child(), condition);
+            }
             return filter;
         }
+
+        private static Expression foldBinaryLogic(Expression expression) {
+            if (expression instanceof Or) {
+                Or or = (Or) expression;
+                boolean nullLeft = Expressions.isNull(or.left());
+                boolean nullRight = Expressions.isNull(or.right());
+                if (nullLeft && nullRight) {
+                    return Literal.NULL;
+                }
+                if (nullLeft) {
+                    return or.right();
+                }
+                if (nullRight) {
+                    return or.left();
+                }
+            }
+            if (expression instanceof And) {
+                And and = (And) expression;
+                if (Expressions.isNull(and.left()) || Expressions.isNull(and.right())) {
+                    return Literal.NULL;
+                }
+            }
+            return expression;
+        }
     }
 
     static class ReplaceAliasesInHaving extends OptimizerRule<Filter> {
@@ -1130,21 +1159,18 @@ public class Optimizer extends RuleExecutor<LogicalPlan> {
                 if (((IsNotNull) e).field().nullable() == false) {
                     return new Literal(e.location(), Expressions.name(e), Boolean.TRUE, DataType.BOOLEAN);
                 }
-            }
-            // see https://github.com/elastic/elasticsearch/issues/34876
-            // similar for IsNull once it gets introduced
+                // see https://github.com/elastic/elasticsearch/issues/34876
+                // similar for IsNull once it gets introduced
 
-            if (e instanceof In) {
+            } else if (e instanceof In) {
                 In in = (In) e;
                 if (Expressions.isNull(in.value())) {
                     return Literal.of(in, null);
                 }
-            }
 
-            if (e.nullable() && Expressions.anyMatch(e.children(), Expressions::isNull)) {
+            } else if (e.nullable() && Expressions.anyMatch(e.children(), Expressions::isNull)) {
                 return Literal.of(e, null);
             }
-
             return e;
         }
     }
@@ -1961,4 +1987,4 @@ public class Optimizer extends RuleExecutor<LogicalPlan> {
     enum TransformDirection {
         UP, DOWN
     };
-}
+}

+ 46 - 0
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

@@ -65,6 +65,52 @@ public class QueryFolderTests extends ESTestCase {
         assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
     }
 
+    public void testFoldingToLocalExecBooleanAndNull_WhereClause2() {
+        PhysicalPlan p = plan("SELECT true OR null");
+    }
+    public void testFoldingToLocalExecBooleanAndNull_WhereClause() {
+        PhysicalPlan p = plan("SELECT keyword FROM test WHERE int > 10 AND null AND true");
+        assertEquals(LocalExec.class, p.getClass());
+        LocalExec le = (LocalExec) p;
+        assertEquals(EmptyExecutable.class, le.executable().getClass());
+        EmptyExecutable ee = (EmptyExecutable) le.executable();
+        assertEquals(1, ee.output().size());
+        assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
+    }
+
+    public void testFoldingToLocalExecBooleanAndNull_HavingClause() {
+        PhysicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) > 10 AND null");
+        assertEquals(LocalExec.class, p.getClass());
+        LocalExec le = (LocalExec) p;
+        assertEquals(EmptyExecutable.class, le.executable().getClass());
+        EmptyExecutable ee = (EmptyExecutable) le.executable();
+        assertEquals(2, ee.output().size());
+        assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
+        assertThat(ee.output().get(1).toString(), startsWith("MAX(int){a->"));
+    }
+
+    public void testFoldingBooleanOrNull_WhereClause() {
+        PhysicalPlan p = plan("SELECT keyword FROM test WHERE int > 10 OR null OR false");
+        assertEquals(EsQueryExec.class, p.getClass());
+        EsQueryExec ee = (EsQueryExec) p;
+        assertEquals("{\"range\":{\"int\":{\"from\":10,\"to\":null,\"include_lower\":false,\"include_upper\":false,\"boost\":1.0}}}",
+            ee.queryContainer().query().asBuilder().toString().replaceAll("\\s+", ""));
+        assertEquals(1, ee.output().size());
+        assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
+    }
+
+    public void testFoldingBooleanOrNull_HavingClause() {
+        PhysicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) > 10 OR null");
+        assertEquals(EsQueryExec.class, p.getClass());
+        EsQueryExec ee = (EsQueryExec) p;
+        assertTrue(ee.queryContainer().aggs().asAggBuilder().toString().replaceAll("\\s+", "").contains(
+            "\"script\":{\"source\":\"InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.gt(params.a0,params.v0))\"," +
+            "\"lang\":\"painless\",\"params\":{\"v0\":10}},"));
+        assertEquals(2, ee.output().size());
+        assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
+        assertThat(ee.output().get(1).toString(), startsWith("MAX(int){a->"));
+    }
+
     public void testFoldingOfIsNotNull() {
         PhysicalPlan p = plan("SELECT keyword FROM test WHERE (keyword IS NULL) IS NOT NULL");
         assertEquals(EsQueryExec.class, p.getClass());