From 65717f6f420b2e3afd0dc089ae490d4aa1d14e90 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 11 Oct 2019 19:10:01 +0200 Subject: [PATCH] SQL: Fix Nullability of DATEADD (#47921) Previously, Nullability was set to UNKNOWN instead of TRUE which resulted on QueryFolder not correctly folding to NULL if any of the args was null. Remove the overriding nullable() also for DatePart/DateTrunc to allow delegation the parent class. (cherry picked from commit 05a7108e133b5ae7bec2257db5ae2d30ad926ee2) --- .../datetime/BinaryDateTimeFunction.java | 6 -- .../function/scalar/datetime/DateAdd.java | 6 -- .../function/scalar/datetime/DatePart.java | 6 -- .../function/scalar/datetime/DateTrunc.java | 6 -- .../datetime/ThreeArgsDateTimeFunction.java | 6 -- .../xpack/sql/optimizer/OptimizerTests.java | 62 ++++++++++++++----- 6 files changed, 45 insertions(+), 47 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java index 02ebfc648f4..f0583f57a5e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BinaryDateTimeFunction.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; -import org.elasticsearch.xpack.sql.expression.Nullability; import org.elasticsearch.xpack.sql.expression.function.scalar.BinaryScalarFunction; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; @@ -79,11 +78,6 @@ public abstract class BinaryDateTimeFunction extends BinaryScalarFunction { protected abstract Pipe createPipe(Pipe left, Pipe right, ZoneId zoneId); - @Override - public Nullability nullable() { - return Nullability.TRUE; - } - @Override protected ScriptTemplate asScriptFrom(ScriptTemplate leftScript, ScriptTemplate rightScript) { return new ScriptTemplate( diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateAdd.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateAdd.java index b7d3c522d9d..a24701f7882 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateAdd.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateAdd.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; -import org.elasticsearch.xpack.sql.expression.Nullability; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.tree.Source; @@ -136,11 +135,6 @@ public class DateAdd extends ThreeArgsDateTimeFunction { return NodeInfo.create(this, DateAdd::new, first(), second(), third(), zoneId()); } - @Override - public Nullability nullable() { - return Nullability.UNKNOWN; - } - @Override protected Pipe createPipe(Pipe first, Pipe second, Pipe third, ZoneId zoneId) { return new DateAddPipe(source(), this, first, second, third, zoneId); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java index b6a7c28ac1a..37dc1b5ca6b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; -import org.elasticsearch.xpack.sql.expression.Nullability; import org.elasticsearch.xpack.sql.expression.function.scalar.BinaryScalarFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor.DateTimeExtractor; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; @@ -97,11 +96,6 @@ public class DatePart extends BinaryDateTimeFunction { return NodeInfo.create(this, DatePart::new, left(), right(), zoneId()); } - @Override - public Nullability nullable() { - return Nullability.TRUE; - } - @Override protected String scriptMethodName() { return "datePart"; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java index 2cc6e527428..d8029df191d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; -import org.elasticsearch.xpack.sql.expression.Nullability; import org.elasticsearch.xpack.sql.expression.function.scalar.BinaryScalarFunction; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.tree.NodeInfo; @@ -153,11 +152,6 @@ public class DateTrunc extends BinaryDateTimeFunction { return NodeInfo.create(this, DateTrunc::new, left(), right(), zoneId()); } - @Override - public Nullability nullable() { - return Nullability.TRUE; - } - @Override protected String scriptMethodName() { return "dateTrunc"; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/ThreeArgsDateTimeFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/ThreeArgsDateTimeFunction.java index 1be010f32d3..6b7c0c30ae7 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/ThreeArgsDateTimeFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/ThreeArgsDateTimeFunction.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; -import org.elasticsearch.xpack.sql.expression.Nullability; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; @@ -69,11 +68,6 @@ public abstract class ThreeArgsDateTimeFunction extends ScalarFunction { protected abstract Pipe createPipe(Pipe first, Pipe second, Pipe third, ZoneId zoneId); - @Override - public Nullability nullable() { - return Nullability.TRUE; - } - @Override public boolean foldable() { return first().foldable() && second().foldable() && third().foldable(); 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 96222016c97..68dbf0bbc92 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 @@ -35,6 +35,9 @@ import org.elasticsearch.xpack.sql.expression.function.aggregate.Sum; import org.elasticsearch.xpack.sql.expression.function.aggregate.SumOfSquares; import org.elasticsearch.xpack.sql.expression.function.aggregate.VarPop; import org.elasticsearch.xpack.sql.expression.function.scalar.Cast; +import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateAdd; +import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DatePart; +import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTrunc; import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DayName; import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DayOfMonth; import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DayOfYear; @@ -614,7 +617,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(TWO, e.children().get(1)); assertEquals(DataType.INTEGER, e.dataType()); } - + public void testConcatFoldingIsNotNull() { FoldNull foldNull = new FoldNull(); assertEquals(1, foldNull.rule(new Concat(EMPTY, NULL, ONE)).fold()); @@ -622,6 +625,31 @@ public class OptimizerTests extends ESTestCase { assertEquals(StringUtils.EMPTY, foldNull.rule(new Concat(EMPTY, NULL, NULL)).fold()); } + public void testFoldNullDateAdd() { + FoldNull foldNull = new FoldNull(); + assertNullLiteral(foldNull.rule(new DateAdd(EMPTY, NULL, TWO, THREE, UTC))); + assertNullLiteral(foldNull.rule(new DateAdd(EMPTY, ONE, NULL, THREE, UTC))); + assertNullLiteral(foldNull.rule(new DateAdd(EMPTY, ONE, TWO, NULL, UTC))); + assertNullLiteral(foldNull.rule(new DateAdd(EMPTY, NULL, NULL, NULL, UTC))); + assertTrue(foldNull.rule(new DateAdd(EMPTY, ONE, TWO, THREE, UTC)) instanceof DateAdd); + } + + public void testFoldNullDatePart() { + FoldNull foldNull = new FoldNull(); + assertNullLiteral(foldNull.rule(new DatePart(EMPTY, NULL, TWO, UTC))); + assertNullLiteral(foldNull.rule(new DatePart(EMPTY, ONE, NULL, UTC))); + assertNullLiteral(foldNull.rule(new DatePart(EMPTY, NULL, NULL, UTC))); + assertTrue(foldNull.rule(new DatePart(EMPTY, ONE, TWO, UTC)) instanceof DatePart); + } + + public void testFoldNullDateTrunc() { + FoldNull foldNull = new FoldNull(); + assertNullLiteral(foldNull.rule(new DateTrunc(EMPTY, NULL, TWO, UTC))); + assertNullLiteral(foldNull.rule(new DateTrunc(EMPTY, ONE, NULL, UTC))); + assertNullLiteral(foldNull.rule(new DateTrunc(EMPTY, NULL, NULL, UTC))); + assertTrue(foldNull.rule(new DateTrunc(EMPTY, ONE, TWO, UTC)) instanceof DateTrunc); + } + public void testSimplifyCaseConditionsFoldWhenFalse() { // CASE WHEN a = 1 THEN 'foo1' // WHEN 1 = 2 THEN 'bar1' @@ -1453,7 +1481,7 @@ public class OptimizerTests extends ESTestCase { assertSame(last, aggregates.get(0)); assertEquals(max2, aggregates.get(1)); } - + public void testSortAggregateOnOrderByWithTwoFields() { FieldAttribute firstField = new FieldAttribute(EMPTY, "first_field", new EsField("first_field", DataType.BYTE, emptyMap(), true)); FieldAttribute secondField = new FieldAttribute(EMPTY, "second_field", @@ -1462,12 +1490,12 @@ public class OptimizerTests extends ESTestCase { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstField, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondField, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondField, firstField), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -1475,7 +1503,7 @@ public class OptimizerTests extends ESTestCase { assertTrue(order.get(1).child() instanceof FieldAttribute); assertEquals("first_field", ((FieldAttribute) order.get(0).child()).name()); assertEquals("second_field", ((FieldAttribute) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -1485,7 +1513,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(firstField, groupings.get(0)); assertEquals(secondField, groupings.get(1)); } - + public void testSortAggregateOnOrderByOnlyAliases() { FieldAttribute firstField = new FieldAttribute(EMPTY, "first_field", new EsField("first_field", DataType.BYTE, emptyMap(), true)); FieldAttribute secondField = new FieldAttribute(EMPTY, "second_field", @@ -1494,12 +1522,12 @@ public class OptimizerTests extends ESTestCase { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstAlias, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondAlias, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondAlias, firstAlias), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -1507,7 +1535,7 @@ public class OptimizerTests extends ESTestCase { assertTrue(order.get(1).child() instanceof Alias); assertEquals("first_alias", ((Alias) order.get(0).child()).name()); assertEquals("second_alias", ((Alias) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -1536,7 +1564,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(column, in.value()); assertEquals(Arrays.asList(L(1), L(2)), in.list()); } - + /** * Test queries like SELECT MIN(agg_field), MAX(agg_field) FROM table WHERE MATCH(match_field,'A') AND/OR QUERY('match_field:A') * or SELECT STDDEV_POP(agg_field), VAR_POP(agg_field) FROM table WHERE MATCH(match_field,'A') AND/OR QUERY('match_field:A') @@ -1544,7 +1572,7 @@ public class OptimizerTests extends ESTestCase { public void testAggregatesPromoteToStats_WithFullTextPredicatesConditions() { FieldAttribute matchField = new FieldAttribute(EMPTY, "match_field", new EsField("match_field", DataType.TEXT, emptyMap(), true)); FieldAttribute aggField = new FieldAttribute(EMPTY, "agg_field", new EsField("agg_field", DataType.INTEGER, emptyMap(), true)); - + FullTextPredicate matchPredicate = new MatchQueryPredicate(EMPTY, matchField, "A", StringUtils.EMPTY); FullTextPredicate multiMatchPredicate = new MultiMatchQueryPredicate(EMPTY, "match_field", "A", StringUtils.EMPTY); FullTextPredicate stringQueryPredicate = new StringQueryPredicate(EMPTY, "match_field:A", StringUtils.EMPTY); @@ -1552,12 +1580,12 @@ public class OptimizerTests extends ESTestCase { FullTextPredicate left = randomFrom(predicates); FullTextPredicate right = randomFrom(predicates); - + BinaryLogic or = new Or(EMPTY, left, right); BinaryLogic and = new And(EMPTY, left, right); BinaryLogic condition = randomFrom(or, and); Filter filter = new Filter(EMPTY, FROM(), condition); - + List aggregates; boolean isSimpleStats = randomBoolean(); if (isSimpleStats) { @@ -1576,13 +1604,13 @@ public class OptimizerTests extends ESTestCase { } else { result = new ReplaceAggsWithExtendedStats().apply(aggregatePlan); } - + assertTrue(result instanceof Aggregate); Aggregate resultAgg = (Aggregate) result; assertEquals(2, resultAgg.aggregates().size()); assertTrue(resultAgg.aggregates().get(0) instanceof InnerAggregate); assertTrue(resultAgg.aggregates().get(1) instanceof InnerAggregate); - + InnerAggregate resultFirstAgg = (InnerAggregate) resultAgg.aggregates().get(0); InnerAggregate resultSecondAgg = (InnerAggregate) resultAgg.aggregates().get(1); assertEquals(resultFirstAgg.inner(), firstAggregate); @@ -1598,8 +1626,8 @@ public class OptimizerTests extends ESTestCase { assertEquals(((ExtendedStats) resultFirstAgg.outer()).field(), aggField); assertEquals(((ExtendedStats) resultSecondAgg.outer()).field(), aggField); } - + assertTrue(resultAgg.child() instanceof Filter); assertEquals(resultAgg.child(), filter); } -} \ No newline at end of file +}