From c9a84fe361220c379f19e458ee8a9e49078247d4 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 8 Nov 2018 23:32:59 +0100 Subject: [PATCH] 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 --- .../predicate/logical/BinaryLogic.java | 3 +- .../xpack/sql/optimizer/Optimizer.java | 48 ++++++++++++++----- .../xpack/sql/planner/QueryFolderTests.java | 46 ++++++++++++++++++ 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/BinaryLogic.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/BinaryLogic.java index 93c50fbc135..5e4175797eb 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/logical/BinaryLogic.java +++ b/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 { @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 { @@ -1130,21 +1159,18 @@ public class Optimizer extends RuleExecutor { 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 { enum TransformDirection { UP, DOWN }; -} \ No newline at end of file +} 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 77606ab1390..0ca0df6fc08 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 @@ -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());