SQL: Fix edge case: <field> IN (null)

Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: elastic#34750
matriv · Oct 24, 2018 · c5c5cfd · c5c5cfd
1 parent 98cd7ca
commit c5c5cfd
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 6 deletions.
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java
@@ -78,11 +78,16 @@ public boolean nullable() {
 
     @Override
     public boolean foldable() {
-        return Expressions.foldable(children());
+        return Expressions.foldable(children()) ||
+            (Expressions.foldable(list) && list().stream().allMatch(e -> e.dataType() == DataType.NULL));
     }
 
     @Override
     public Boolean fold() {
+        if (value.dataType() == DataType.NULL || list.size() == 1 && list.get(0).dataType() == DataType.NULL) {
+            return false;
+        }
+
         Object foldedLeftValue = value.fold();
         Boolean result = false;
         for (Expression rightValue : list) {

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java
@@ -11,23 +11,28 @@
 import org.elasticsearch.xpack.sql.tree.Location;
 import org.elasticsearch.xpack.sql.type.DataType;
 
+import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 
 import static org.elasticsearch.index.query.QueryBuilders.termsQuery;
 
 public class TermsQuery extends LeafQuery {
 
     private final String term;
-    private final LinkedHashSet<Object> values;
+    private final Set<Object> values;
 
     public TermsQuery(Location location, String term, List<Expression> values) {
         super(location);
         this.term = term;
         values.removeIf(e -> e.dataType() == DataType.NULL);
-        this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType()));
-        this.values.removeIf(Objects::isNull);
+        if (values.isEmpty()) {
+            this.values = Collections.emptySet();
+        } else {
+            this.values = new LinkedHashSet<>(Foldables.valuesOf(values, values.get(0).dataType()));
+        }
     }
 
     @Override

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java
@@ -95,7 +95,7 @@ public class OptimizerTests extends ESTestCase {
     private static final Literal FOUR = L(4);
     private static final Literal FIVE = L(5);
     private static final Literal SIX = L(6);
-
+    private static final Literal NULL = L(null);
 
     public static class DummyBooleanExpression extends Expression {
 
@@ -323,6 +323,18 @@ public void testConstantFoldingIn_LeftValueNotFoldable() {
         assertThat(Foldables.valuesOf(in.list(), DataType.INTEGER), contains(1 ,2 ,3 ,4));
     }
 
+    public void testConstantFoldingIn_RightValueIsNull() {
+        In in = new In(EMPTY, getFieldAttribute(), Arrays.asList(NULL, NULL));
+        Literal result= (Literal) new ConstantFolding().rule(in);
+        assertEquals(false, result.value());
+    }
+
+    public void testConstantFoldingIn_LeftValueIsNull() {
+        In in = new In(EMPTY, NULL, Arrays.asList(ONE, TWO, THREE));
+        Literal result= (Literal) new ConstantFolding().rule(in);
+        assertEquals(false, result.value());
+    }
+
     public void testArithmeticFolding() {
         assertEquals(10, foldOperator(new Add(EMPTY, L(7), THREE)));
         assertEquals(4, foldOperator(new Sub(EMPTY, L(7), THREE)));

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java
@@ -64,6 +64,16 @@ public void testFoldingToLocalExecWithProject() {
         assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#"));
     }
 
+    public void testFoldingToLocalExecWithProject_FoldableIn() {
+        PhysicalPlan p = plan("SELECT keyword FROM test WHERE int IN (null, null)");
+        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 testFoldingToLocalExecWithProject_WithOrderAndLimit() {
         PhysicalPlan p = plan("SELECT keyword FROM test WHERE 1 = 2 ORDER BY int LIMIT 10");
         assertEquals(LocalExec.class, p.getClass());

diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -173,7 +173,7 @@ public void testTranslateInExpression_WhereClause() throws IOException {
         assertEquals("keyword:(bar foo lala)", tq.asBuilder().toQuery(createShardContext()).toString());
     }
 
-    public void testTranslateInExpression_WhereClauseAndNullHAndling() throws IOException {
+    public void testTranslateInExpression_WhereClauseAndNullHandling() throws IOException {
         LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', null, 'lala', null, 'foo', concat('la', 'la'))");
         assertTrue(p instanceof Project);
         assertTrue(p.children().get(0) instanceof Filter);