From 960a674442a3423b66d3e5816b860c5dff62bce7 Mon Sep 17 00:00:00 2001 From: Sree Charan Manamala <155449160+sreemanamala@users.noreply.github.com> Date: Thu, 18 Apr 2024 15:47:13 +0530 Subject: [PATCH] Corrected Strict NON NULL return type checks (#16279) --- .../org/apache/druid/math/expr/Function.java | 4 +- .../apache/druid/math/expr/FunctionTest.java | 2 + .../builtin/ContainsOperatorConversion.java | 2 +- .../builtin/RegexpLikeOperatorConversion.java | 2 +- .../builtin/SubstringOperatorConversion.java | 3 +- .../TimeInIntervalConvertletFactory.java | 4 +- .../druid/sql/calcite/CalciteQueryTest.java | 74 +++++++++++++++++++ 7 files changed, 84 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/math/expr/Function.java b/processing/src/main/java/org/apache/druid/math/expr/Function.java index e870541c909..4c073114286 100644 --- a/processing/src/main/java/org/apache/druid/math/expr/Function.java +++ b/processing/src/main/java/org/apache/druid/math/expr/Function.java @@ -3606,7 +3606,7 @@ public interface Function extends NamedFunction final Object[] array = arrayExpr.asArray(); final int position = scalarExpr.asInt(); - if (array.length > position) { + if (array.length > position && position >= 0) { return ExprEval.ofType(arrayExpr.elementType(), array[position]); } return ExprEval.of(null); @@ -3634,7 +3634,7 @@ public interface Function extends NamedFunction final Object[] array = arrayExpr.asArray(); final int position = scalarExpr.asInt() - 1; - if (array.length > position) { + if (array.length > position && position >= 0) { return ExprEval.ofType(arrayExpr.elementType(), array[position]); } return ExprEval.of(null); diff --git a/processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java b/processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java index fae7bca736f..30d549dc351 100644 --- a/processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java +++ b/processing/src/test/java/org/apache/druid/math/expr/FunctionTest.java @@ -336,6 +336,7 @@ public class FunctionTest extends InitializedNullHandlingTest { assertExpr("array_offset([1, 2, 3], 2)", 3L); assertArrayExpr("array_offset([1, 2, 3], 3)", null); + assertArrayExpr("array_offset([1, 2, 3], -1)", null); assertExpr("array_offset(a, 2)", "baz"); // nested types only work with typed bindings right now, and pretty limited support for stuff assertExpr("array_offset(nestedArray, 1)", ImmutableMap.of("x", 4L, "y", 6.6), typedBindings); @@ -346,6 +347,7 @@ public class FunctionTest extends InitializedNullHandlingTest { assertExpr("array_ordinal([1, 2, 3], 3)", 3L); assertArrayExpr("array_ordinal([1, 2, 3], 4)", null); + assertArrayExpr("array_ordinal([1, 2, 3], 0)", null); assertExpr("array_ordinal(a, 3)", "baz"); // nested types only work with typed bindings right now, and pretty limited support for stuff assertExpr("array_ordinal(nestedArray, 2)", ImmutableMap.of("x", 4L, "y", 6.6), typedBindings); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ContainsOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ContainsOperatorConversion.java index 5e8071fb90c..c13a95202e4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ContainsOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ContainsOperatorConversion.java @@ -82,7 +82,7 @@ public class ContainsOperatorConversion extends DirectOperatorConversion .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER) .requiredOperandCount(2) .literalOperands(1) - .returnTypeNonNull(SqlTypeName.BOOLEAN) + .returnTypeNullable(SqlTypeName.BOOLEAN) .functionCategory(SqlFunctionCategory.STRING) .build(); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpLikeOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpLikeOperatorConversion.java index cc677215cba..1a2013109e8 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpLikeOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpLikeOperatorConversion.java @@ -46,7 +46,7 @@ public class RegexpLikeOperatorConversion implements SqlOperatorConversion .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.CHARACTER) .requiredOperandCount(2) .literalOperands(1) - .returnTypeNonNull(SqlTypeName.BOOLEAN) + .returnTypeCascadeNullable(SqlTypeName.BOOLEAN) .functionCategory(SqlFunctionCategory.STRING) .build(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SubstringOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SubstringOperatorConversion.java index 41f97b9322d..b03a74d9fbb 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SubstringOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SubstringOperatorConversion.java @@ -28,6 +28,7 @@ import org.apache.calcite.sql.SqlFunctionCategory; import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.type.ReturnTypes; import org.apache.calcite.sql.type.SqlTypeFamily; +import org.apache.calcite.sql.type.SqlTypeTransforms; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.query.extraction.SubstringDimExtractionFn; import org.apache.druid.segment.column.RowSignature; @@ -45,7 +46,7 @@ public class SubstringOperatorConversion implements SqlOperatorConversion .operatorBuilder("SUBSTRING") .operandTypes(SqlTypeFamily.CHARACTER, SqlTypeFamily.INTEGER, SqlTypeFamily.INTEGER) .functionCategory(SqlFunctionCategory.STRING) - .returnTypeInference(ReturnTypes.ARG0) + .returnTypeInference(ReturnTypes.ARG0.andThen(SqlTypeTransforms.FORCE_NULLABLE)) .requiredOperandCount(2) .build(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/TimeInIntervalConvertletFactory.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/TimeInIntervalConvertletFactory.java index e3dcf71879a..fbf24993c37 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/TimeInIntervalConvertletFactory.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/TimeInIntervalConvertletFactory.java @@ -27,8 +27,8 @@ import org.apache.calcite.sql.SqlFunctionCategory; import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.type.ReturnTypes; import org.apache.calcite.sql.type.SqlTypeFamily; -import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql2rel.SqlRexContext; import org.apache.calcite.sql2rel.SqlRexConvertlet; import org.apache.calcite.util.Static; @@ -56,7 +56,7 @@ public class TimeInIntervalConvertletFactory implements DruidConvertletFactory .operandTypes(SqlTypeFamily.TIMESTAMP, SqlTypeFamily.CHARACTER) .requiredOperandCount(2) .literalOperands(1) - .returnTypeNonNull(SqlTypeName.BOOLEAN) + .returnTypeInference(ReturnTypes.BOOLEAN_NULLABLE) .functionCategory(SqlFunctionCategory.TIMEDATE) .build(); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 9c51c70d05e..d851ee027a9 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -34,6 +34,7 @@ import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.UOE; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.common.granularity.PeriodGranularity; +import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.Druids; import org.apache.druid.query.InlineDataSource; @@ -110,6 +111,7 @@ import org.apache.druid.segment.VirtualColumns; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.segment.join.JoinType; +import org.apache.druid.segment.virtual.ExpressionVirtualColumn; import org.apache.druid.sql.calcite.DecoupledTestConfig.NativeQueryIgnore; import org.apache.druid.sql.calcite.NotYetSupported.Modes; import org.apache.druid.sql.calcite.expression.DruidExpression; @@ -6120,6 +6122,34 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ); } + @Test + public void testTimeInIntervalBooleanNullable() + { + testQuery( + "SELECT TIME_IN_INTERVAL(TIME_PARSE('2000-01-10'), '2000-01-01/P1Y')", + QUERY_CONTEXT_LOS_ANGELES, + ImmutableList.of( + Druids.newScanQueryBuilder() + .dataSource(InlineDataSource.fromIterable( + ImmutableList.of(new Object[]{0L}), + RowSignature.builder() + .add("ZERO", ColumnType.LONG) + .build() + )) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns(new ExpressionVirtualColumn("v0", ExprEval.of(1L).toExpr(), ColumnType.LONG)) + .columns("v0") + .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .context(QUERY_CONTEXT_LOS_ANGELES) + .build() + ), + ImmutableList.of( + new Object[]{true} + ) + ); + } + @Test public void testCountStarWithTimeInIntervalFilterNonLiteral() { @@ -15612,4 +15642,48 @@ public class CalciteQueryTest extends BaseCalciteQueryTest ) ); } + + @Test + public void testStringOperationsNullableInference() + { + testBuilder() + .sql( + "SELECT ICONTAINS_STRING(dim3, 'a'), REGEXP_LIKE(dim3,'x'), SUBSTRING(dim3, 1, 1) " + + "from druid.numfoo where dim3 is NULL LIMIT 1" + ) + .queryContext(QUERY_CONTEXT_LOS_ANGELES) + .expectedQueries( + ImmutableList.of( + Druids.newScanQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns( + new ExpressionVirtualColumn( + "v0", + NullHandling.replaceWithDefault() ? "0" : "null", + ColumnType.LONG, + ExprMacroTable.nil() + ), + new ExpressionVirtualColumn( + "v1", + "null", + ColumnType.STRING, + ExprMacroTable.nil() + ) + ) + .columns("v0", "v1") + .filters(isNull("dim3")) + .limit(1) + .resultFormat(ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .context(QUERY_CONTEXT_LOS_ANGELES) + .build() + ) + ).expectedResults( + ResultMatchMode.RELAX_NULLS, + ImmutableList.of( + new Object[]{null, null, null} + ) + ); + } }