From bd143334d3ec773d5e47090233dd6218e4e4bf98 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 25 Oct 2018 14:24:11 +0200 Subject: [PATCH] SQL: Fix edge case: ` IN (null)` (#34802) Handle the case when `null` is the only value in the list so that it's translated to a `MatchNoDocsQuery`. Followup to: #34750 --- .../xpack/sql/expression/predicate/In.java | 10 +++++++++- .../xpack/sql/querydsl/query/TermsQuery.java | 11 ++++++++--- .../xpack/sql/optimizer/OptimizerTests.java | 14 +++++++++++++- .../xpack/sql/planner/QueryFolderTests.java | 10 ++++++++++ .../xpack/sql/planner/QueryTranslatorTests.java | 2 +- 5 files changed, 41 insertions(+), 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 index 4ce1088f806..9b16b77511c 100644 --- 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,19 @@ public class In extends NamedExpression implements ScriptWeaver { @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) { + return null; + } + if (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 index 4366e2d404c..91ea49a8a3c 100644 --- 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.expression.Foldables; 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 values; + private final Set values; public TermsQuery(Location location, String term, List 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 index acd0378ee01..137f7b68d7a 100644 --- 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 class OptimizerTests extends ESTestCase { 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); + assertNull(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 index b6643fb7d47..5fac14e2397 100644 --- 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 class QueryFolderTests extends AbstractBuilderTestCase { 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/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 95b9be33a12..c1e5a0d2daf 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -173,7 +173,7 @@ public class QueryTranslatorTests extends AbstractBuilderTestCase { 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);