From 806649f8af6110c4a60b3014c9014ef4e012008c Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 16 Aug 2024 22:41:12 -0700 Subject: [PATCH] SQL: Fix nullable DATE, TIMESTAMP reduction. (#16915) Reduction of nullable DATE and TIMESTAMP expressions did not perform a necessary null check, so would in some cases reduce to 1970-01-01 00:00:00 (epoch) rather than NULL. --- .../sql/calcite/planner/DruidRexExecutor.java | 56 ++++++++------- .../calcite/planner/DruidRexExecutorTest.java | 71 +++++++++++++++++++ 2 files changed, 103 insertions(+), 24 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java index 715c5caaadc..1f73834fdb2 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java @@ -90,33 +90,41 @@ public class DruidRexExecutor implements RexExecutor literal = rexBuilder.makeLiteral(exprResult.asBoolean(), constExp.getType(), true); } } else if (sqlTypeName == SqlTypeName.DATE) { - // It is possible for an expression to have a non-null String value but it can return null when parsed - // as a primitive long/float/double. - // ExprEval.isNumericNull checks whether the parsed primitive value is null or not. - if (!constExp.getType().isNullable() && exprResult.isNumericNull()) { - throw InvalidSqlInput.exception("Illegal DATE constant [%s]", constExp); + if (exprResult.isNumericNull()) { + if (constExp.getType().isNullable()) { + literal = rexBuilder.makeNullLiteral(constExp.getType()); + } else { + // There can be implicit casts of VARCHAR to TIMESTAMP where the VARCHAR is an invalid timestamp, but the + // TIMESTAMP type is not nullable. In this case it's best to throw an error, since it likely means the + // user's SQL query contains an invalid literal. + throw InvalidSqlInput.exception("Illegal DATE constant [%s]", constExp); + } + } else { + literal = rexBuilder.makeDateLiteral( + Calcites.jodaToCalciteDateString( + DateTimes.utc(exprResult.asLong()), + plannerContext.getTimeZone() + ) + ); } - - literal = rexBuilder.makeDateLiteral( - Calcites.jodaToCalciteDateString( - DateTimes.utc(exprResult.asLong()), - plannerContext.getTimeZone() - ) - ); } else if (sqlTypeName == SqlTypeName.TIMESTAMP) { - // It is possible for an expression to have a non-null String value but it can return null when parsed - // as a primitive long/float/double. - // ExprEval.isNumericNull checks whether the parsed primitive value is null or not. - if (!constExp.getType().isNullable() && exprResult.isNumericNull()) { - throw InvalidSqlInput.exception("Illegal TIMESTAMP constant [%s]", constExp); + if (exprResult.isNumericNull()) { + if (constExp.getType().isNullable()) { + literal = rexBuilder.makeNullLiteral(constExp.getType()); + } else { + // There can be implicit casts of VARCHAR to TIMESTAMP where the VARCHAR is an invalid timestamp, but the + // TIMESTAMP type is not nullable. In this case it's best to throw an error, since it likely means the + // user's SQL query contains an invalid literal. + throw InvalidSqlInput.exception("Illegal TIMESTAMP constant [%s]", constExp); + } + } else { + literal = Calcites.jodaToCalciteTimestampLiteral( + rexBuilder, + DateTimes.utc(exprResult.asLong()), + plannerContext.getTimeZone(), + constExp.getType().getPrecision() + ); } - - literal = Calcites.jodaToCalciteTimestampLiteral( - rexBuilder, - DateTimes.utc(exprResult.asLong()), - plannerContext.getTimeZone(), - constExp.getType().getPrecision() - ); } else if (SqlTypeName.NUMERIC_TYPES.contains(sqlTypeName)) { final BigDecimal bigDecimal; diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidRexExecutorTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidRexExecutorTest.java index 072bdd6b6e8..7888ec8090b 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidRexExecutorTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidRexExecutorTest.java @@ -37,6 +37,7 @@ import org.apache.calcite.sql.type.BasicSqlType; import org.apache.calcite.sql.type.SqlTypeFactoryImpl; import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.segment.column.RowSignature; @@ -46,6 +47,7 @@ import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; import org.apache.druid.sql.calcite.expression.OperatorConversions; import org.apache.druid.sql.calcite.expression.builtin.MultiValueStringOperatorConversions; +import org.apache.druid.sql.calcite.expression.builtin.TimeParseOperatorConversion; import org.apache.druid.sql.calcite.schema.DruidSchema; import org.apache.druid.sql.calcite.schema.DruidSchemaCatalog; import org.apache.druid.sql.calcite.schema.NamedDruidSchema; @@ -57,6 +59,7 @@ import org.apache.druid.sql.calcite.util.CalciteTests; import org.apache.druid.sql.hook.DruidHookDispatcher; import org.apache.druid.testing.InitializedNullHandlingTest; import org.easymock.EasyMock; +import org.joda.time.DateTimeZone; import org.junit.Assert; import org.junit.Test; @@ -139,6 +142,74 @@ public class DruidRexExecutorTest extends InitializedNullHandlingTest Assert.assertEquals(new BigDecimal(30L), ((RexLiteral) reduced.get(0)).getValue()); } + @Test + public void testCastDateReduced() + { + // CAST('2010-01-01' AS DATE) + RexNode call = rexBuilder.makeCall( + rexBuilder.getTypeFactory().createSqlType(SqlTypeName.DATE), + SqlStdOperatorTable.CAST, + Collections.singletonList(rexBuilder.makeLiteral("2010-01-01")) + ); + + DruidRexExecutor rexy = new DruidRexExecutor(PLANNER_CONTEXT); + List reduced = new ArrayList<>(); + rexy.reduce(rexBuilder, ImmutableList.of(call), reduced); + Assert.assertEquals(1, reduced.size()); + Assert.assertEquals(SqlKind.LITERAL, reduced.get(0).getKind()); + Assert.assertEquals( + rexBuilder.makeDateLiteral( + Calcites.jodaToCalciteDateString( + DateTimes.of("2010-01-01"), + DateTimeZone.UTC + ) + ), + reduced.get(0) + ); + } + + @Test + public void testTimeParseReduced() + { + // TIME_PARSE('2010-01-01T02:03:04Z') + RexNode call = rexBuilder.makeCall( + new TimeParseOperatorConversion().calciteOperator(), + rexBuilder.makeLiteral("2010-01-01T02:03:04Z") + ); + + DruidRexExecutor rexy = new DruidRexExecutor(PLANNER_CONTEXT); + List reduced = new ArrayList<>(); + rexy.reduce(rexBuilder, ImmutableList.of(call), reduced); + Assert.assertEquals(1, reduced.size()); + Assert.assertEquals(SqlKind.LITERAL, reduced.get(0).getKind()); + Assert.assertEquals( + Calcites.jodaToCalciteTimestampLiteral( + rexBuilder, + DateTimes.of("2010-01-01T02:03:04Z"), + DateTimeZone.UTC, + DruidTypeSystem.DEFAULT_TIMESTAMP_PRECISION + ), + reduced.get(0) + ); + } + + @Test + public void testTimeParseUnparseableReduced() + { + // TIME_PARSE('not a timestamp') + RexNode call = rexBuilder.makeCall( + new TimeParseOperatorConversion().calciteOperator(), + rexBuilder.makeLiteral("not a timestamp") + ); + + DruidRexExecutor rexy = new DruidRexExecutor(PLANNER_CONTEXT); + List reduced = new ArrayList<>(); + rexy.reduce(rexBuilder, ImmutableList.of(call), reduced); + Assert.assertEquals(1, reduced.size()); + Assert.assertEquals(SqlKind.LITERAL, reduced.get(0).getKind()); + Assert.assertTrue(RexLiteral.isNullLiteral(reduced.get(0))); + } + @Test public void testComplexNotReduced() {