From aa21e9acf7a648362decb31c69b4f0f7753518e8 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Sat, 13 Apr 2024 12:07:12 +0530 Subject: [PATCH 1/5] removed strict boolean check in time in interval --- .../TimeInIntervalConvertletFactory.java | 4 +-- .../druid/sql/calcite/CalciteQueryTest.java | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) 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 e3dcf71879a9..fbf24993c373 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.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 9c51c70d05e2..a53e8feddd5f 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.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.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 void testCountStarWithTimeInIntervalFilterInvalidInterval() ); } + @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() { From 9df4fb0f2dd932c3dcee9e0e197d7febb232e473 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 15 Apr 2024 20:43:22 +0530 Subject: [PATCH 2/5] fix strict non null checks --- .../org/apache/druid/math/expr/Function.java | 2 +- .../builtin/ContainsOperatorConversion.java | 2 +- .../builtin/RegexpLikeOperatorConversion.java | 2 +- .../builtin/SubstringOperatorConversion.java | 3 ++- .../calcite/expression/ExpressionsTest.java | 20 +++++++++++++++++++ 5 files changed, 25 insertions(+), 4 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 e870541c909b..baae828c63ea 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 @@ -3634,7 +3634,7 @@ ExprEval doApply(ExprEval arrayExpr, ExprEval scalarExpr) 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/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 5e8071fb90c1..c13a95202e4d 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 @@ private static SqlFunction createSqlFunction(final String functionName) .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 cc677215cbaa..1a2013109e81 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 41f97b9322d0..b03a74d9fbbf 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.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/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java index 489d7f6eb036..dc062c785b4b 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java @@ -270,6 +270,16 @@ public void testSubstring() makeExpression("substring(\"spacey\", (\"p\" - 1), \"p\")"), "hey" ); + testHelper.testExpressionString( + new SubstringOperatorConversion().calciteOperator(), + ImmutableList.of( + testHelper.makeLiteral(""), + testHelper.makeLiteral(1), + testHelper.makeLiteral(2) + ), + makeExpression("substring('', 0, 2)"), + null + ); } @Test @@ -1602,6 +1612,16 @@ public void testContains() makeExpression(ColumnType.LONG, "(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), 1L ); + + testHelper.testExpressionString( + ContainsOperatorConversion.caseSensitive().calciteOperator(), + ImmutableList.of( + testHelper.makeNullLiteral(SqlTypeName.CHAR), + testHelper.makeLiteral("a") + ), + makeExpression("contains_string(null,'a')"), + NullHandling.replaceWithDefault() ? 0L : null + ); } @Test From 7d408510c313c4456f09a3f2a373a12e277f4b1f Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 17 Apr 2024 13:25:50 +0530 Subject: [PATCH 3/5] tests in calcite layer --- .../org/apache/druid/math/expr/Function.java | 2 +- .../druid/sql/calcite/CalciteQueryTest.java | 47 +++++++++++++++++++ .../calcite/expression/ExpressionsTest.java | 20 -------- 3 files changed, 48 insertions(+), 21 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 baae828c63ea..4c073114286a 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 @@ ExprEval doApply(ExprEval arrayExpr, ExprEval scalarExpr) 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); 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 a53e8feddd5f..f270c9a59d60 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 @@ -15642,4 +15642,51 @@ public void testBitwiseXor() ) ); } + + @Test + public void testStringOperationsBooleanNullable() + { + testQuery( + "SELECT ICONTAINS_STRING(dim3, 'a'), REGEXP_LIKE(dim3,'x'), SUBSTRING(dim3, 1, 1) " + + "from druid.numfoo where dim3 is NULL", + QUERY_CONTEXT_LOS_ANGELES, + 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( + NullHandling.replaceWithDefault() + ? selector("dim3", null) + : NullFilter.forColumn("dim3") + ) + .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .context(QUERY_CONTEXT_LOS_ANGELES) + .build() + ), + NullHandling.replaceWithDefault() ? ImmutableList.of( + new Object[]{false, false, ""}, + new Object[]{false, false, ""}, + new Object[]{false, false, ""} + ) : ImmutableList.of( + new Object[]{null, null, null}, + new Object[]{null, null, null} + ) + ); + } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java index dc062c785b4b..489d7f6eb036 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java @@ -270,16 +270,6 @@ public void testSubstring() makeExpression("substring(\"spacey\", (\"p\" - 1), \"p\")"), "hey" ); - testHelper.testExpressionString( - new SubstringOperatorConversion().calciteOperator(), - ImmutableList.of( - testHelper.makeLiteral(""), - testHelper.makeLiteral(1), - testHelper.makeLiteral(2) - ), - makeExpression("substring('', 0, 2)"), - null - ); } @Test @@ -1612,16 +1602,6 @@ public void testContains() makeExpression(ColumnType.LONG, "(icontains_string(\"spacey\",'There') && ('yes' == 'yes'))"), 1L ); - - testHelper.testExpressionString( - ContainsOperatorConversion.caseSensitive().calciteOperator(), - ImmutableList.of( - testHelper.makeNullLiteral(SqlTypeName.CHAR), - testHelper.makeLiteral("a") - ), - makeExpression("contains_string(null,'a')"), - NullHandling.replaceWithDefault() ? 0L : null - ); } @Test From a21c544fef69f1a560988dc8fc9e5dad54548b97 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 17 Apr 2024 20:33:02 +0530 Subject: [PATCH 4/5] improved test --- .../apache/druid/math/expr/FunctionTest.java | 2 + .../druid/sql/calcite/CalciteQueryTest.java | 81 +++++++++---------- 2 files changed, 41 insertions(+), 42 deletions(-) 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 fae7bca736f0..30d549dc3519 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 void testArrayOffset() { 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 void testArrayOrdinal() { 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/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index f270c9a59d60..8a0f74204e06 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 @@ -15644,49 +15644,46 @@ public void testBitwiseXor() } @Test - public void testStringOperationsBooleanNullable() + public void testStringOperationsNullableInference() { - testQuery( - "SELECT ICONTAINS_STRING(dim3, 'a'), REGEXP_LIKE(dim3,'x'), SUBSTRING(dim3, 1, 1) " + - "from druid.numfoo where dim3 is NULL", - QUERY_CONTEXT_LOS_ANGELES, - 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( - NullHandling.replaceWithDefault() - ? selector("dim3", null) - : NullFilter.forColumn("dim3") - ) - .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) - .legacy(false) - .context(QUERY_CONTEXT_LOS_ANGELES) - .build() - ), - NullHandling.replaceWithDefault() ? ImmutableList.of( - new Object[]{false, false, ""}, - new Object[]{false, false, ""}, - new Object[]{false, false, ""} - ) : ImmutableList.of( - new Object[]{null, null, null}, - new Object[]{null, null, null} + 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} + ) + ); } } From f3cecbe666abbec47707cbf8b3c0e0e09180052a Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 17 Apr 2024 21:16:14 +0530 Subject: [PATCH 5/5] checkstyle --- .../druid/sql/calcite/CalciteQueryTest.java | 76 +++++++++---------- 1 file changed, 38 insertions(+), 38 deletions(-) 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 8a0f74204e06..d851ee027a9b 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 @@ -15647,43 +15647,43 @@ public void testBitwiseXor() 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} - ) - ); + .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} + ) + ); } }