From b0f36c1b89bced172d60d9f2188f2e054b7f2072 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 6 Sep 2024 17:07:32 -0700 Subject: [PATCH] fix bug with CastOperatorConversion with types which cannot be mapped to native druid types (#17011) --- .../math/expr/ExpressionTypeConversion.java | 11 +-- .../druid/math/expr/OutputTypeTest.java | 78 ++++++++++++------- .../builtin/CastOperatorConversion.java | 10 ++- .../druid/sql/calcite/planner/Calcites.java | 3 + .../sql/calcite/CalciteArraysQueryTest.java | 24 +++++- 5 files changed, 83 insertions(+), 43 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java b/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java index efe0328a837..16ae5024830 100644 --- a/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java +++ b/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java @@ -91,10 +91,7 @@ public class ExpressionTypeConversion return type; } if (type.isArray() || other.isArray()) { - if (!Objects.equals(type, other)) { - throw new Types.IncompatibleTypeException(type, other); - } - return type; + return leastRestrictiveType(type, other); } if (type.is(ExprType.COMPLEX) || other.is(ExprType.COMPLEX)) { if (type.getComplexTypeName() == null) { @@ -134,12 +131,8 @@ public class ExpressionTypeConversion if (other == null) { return type; } - // arrays cannot be auto converted if (type.isArray() || other.isArray()) { - if (!Objects.equals(type, other)) { - throw new Types.IncompatibleTypeException(type, other); - } - return type; + return leastRestrictiveType(type, other); } if (type.is(ExprType.COMPLEX) || other.is(ExprType.COMPLEX)) { if (type.getComplexTypeName() == null) { diff --git a/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java b/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java index d313749fef8..4ab7c7be0ed 100644 --- a/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java +++ b/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java @@ -20,13 +20,10 @@ package org.apache.druid.math.expr; import com.google.common.collect.ImmutableMap; -import org.apache.druid.java.util.common.IAE; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.testing.InitializedNullHandlingTest; import org.junit.Assert; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import java.util.Map; @@ -48,9 +45,6 @@ public class OutputTypeTest extends InitializedNullHandlingTest .build() ); - @Rule - public ExpectedException expectedException = ExpectedException.none(); - @Test public void testConstantsAndIdentifiers() { @@ -541,7 +535,6 @@ public class OutputTypeTest extends InitializedNullHandlingTest ExpressionType.DOUBLE, ExpressionTypeConversion.operator(ExpressionType.LONG, ExpressionType.STRING) ); - // unless it is an array, and those have to be the same Assert.assertEquals( ExpressionType.LONG_ARRAY, ExpressionTypeConversion.operator(ExpressionType.LONG_ARRAY, ExpressionType.LONG_ARRAY) @@ -554,7 +547,30 @@ public class OutputTypeTest extends InitializedNullHandlingTest ExpressionType.STRING_ARRAY, ExpressionTypeConversion.operator(ExpressionType.STRING_ARRAY, ExpressionType.STRING_ARRAY) ); - + Assert.assertEquals( + ExpressionType.LONG_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.LONG_ARRAY, ExpressionType.LONG) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.STRING, ExpressionType.LONG_ARRAY) + ); + Assert.assertEquals( + ExpressionType.DOUBLE_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.LONG_ARRAY, ExpressionType.DOUBLE_ARRAY) + ); + Assert.assertEquals( + ExpressionType.DOUBLE_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.LONG, ExpressionType.DOUBLE_ARRAY) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.LONG_ARRAY, ExpressionType.STRING_ARRAY) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.operator(ExpressionType.STRING_ARRAY, ExpressionType.DOUBLE_ARRAY) + ); ExpressionType nested = ExpressionType.fromColumnType(ColumnType.NESTED_DATA); Assert.assertEquals( nested, @@ -619,7 +635,6 @@ public class OutputTypeTest extends InitializedNullHandlingTest ExpressionType.STRING, ExpressionTypeConversion.function(ExpressionType.STRING, ExpressionType.STRING) ); - // unless it is an array, and those have to be the same Assert.assertEquals( ExpressionType.LONG_ARRAY, ExpressionTypeConversion.function(ExpressionType.LONG_ARRAY, ExpressionType.LONG_ARRAY) @@ -632,6 +647,30 @@ public class OutputTypeTest extends InitializedNullHandlingTest ExpressionType.STRING_ARRAY, ExpressionTypeConversion.function(ExpressionType.STRING_ARRAY, ExpressionType.STRING_ARRAY) ); + Assert.assertEquals( + ExpressionType.DOUBLE_ARRAY, + ExpressionTypeConversion.function(ExpressionType.DOUBLE_ARRAY, ExpressionType.LONG_ARRAY) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.function(ExpressionType.DOUBLE_ARRAY, ExpressionType.STRING_ARRAY) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.function(ExpressionType.STRING_ARRAY, ExpressionType.LONG_ARRAY) + ); + Assert.assertEquals( + ExpressionType.STRING_ARRAY, + ExpressionTypeConversion.function(ExpressionType.STRING, ExpressionType.LONG_ARRAY) + ); + Assert.assertEquals( + ExpressionType.DOUBLE_ARRAY, + ExpressionTypeConversion.function(ExpressionType.LONG_ARRAY, ExpressionType.DOUBLE_ARRAY) + ); + Assert.assertEquals( + ExpressionType.DOUBLE_ARRAY, + ExpressionTypeConversion.function(ExpressionType.LONG, ExpressionType.DOUBLE_ARRAY) + ); ExpressionType nested = ExpressionType.fromColumnType(ColumnType.NESTED_DATA); Assert.assertEquals( nested, @@ -719,27 +758,6 @@ public class OutputTypeTest extends InitializedNullHandlingTest ); } - @Test - public void testAutoConversionArrayMismatchArrays() - { - expectedException.expect(IAE.class); - ExpressionTypeConversion.function(ExpressionType.DOUBLE_ARRAY, ExpressionType.LONG_ARRAY); - } - - @Test - public void testAutoConversionArrayMismatchArrayScalar() - { - expectedException.expect(IAE.class); - ExpressionTypeConversion.function(ExpressionType.DOUBLE_ARRAY, ExpressionType.LONG); - } - - @Test - public void testAutoConversionArrayMismatchScalarArray() - { - expectedException.expect(IAE.class); - ExpressionTypeConversion.function(ExpressionType.DOUBLE, ExpressionType.LONG_ARRAY); - } - private void assertOutputType(String expression, Expr.InputBindingInspector inspector, ExpressionType outputType) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil(), false); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java index ac34abe48d1..d82d3ea9546 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java @@ -93,11 +93,17 @@ public class CastOperatorConversion implements SqlOperatorConversion : ExpressionType.fromColumnType(toDruidType); if (toExpressionType == null) { - // We have no runtime type for these SQL types. + // We have no runtime type for to SQL type. return null; } if (fromExpressionType == null) { - return DruidExpression.ofLiteral(toDruidType, DruidExpression.nullLiteral()); + // Calcites.getColumnTypeForRelDataType returns null in cases of NULL, but also any type which cannot be + // mapped to a native druid type. in the case of the former, make a null literal of the toType + if (fromType.equals(SqlTypeName.NULL)) { + return DruidExpression.ofLiteral(toDruidType, DruidExpression.nullLiteral()); + } + // otherwise, we have no runtime type for from SQL type. + return null; } final DruidExpression typeCastExpression; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java index 64929c6445e..6310b23543d 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java @@ -218,6 +218,9 @@ public class Calcites if (elementType != null) { return ColumnType.ofArray(elementType); } + if (type.getComponentType().getSqlTypeName() == SqlTypeName.NULL) { + return ColumnType.LONG_ARRAY; + } return null; } else { return null; diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java index 2cd798e193a..d3453404cd9 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java @@ -116,8 +116,6 @@ public class CalciteArraysQueryTest extends BaseCalciteQueryTest } } - // test some query stuffs, sort of limited since no native array column types so either need to use constructor or - // array aggregator @Test public void testSelectConstantArrayExpressionFromTable() { @@ -7478,4 +7476,26 @@ public class CalciteArraysQueryTest extends BaseCalciteQueryTest ) ); } + + @Test + public void testNullArray() + { + testQuery( + "SELECT arrayLongNulls = ARRAY[null, null] FROM druid.arrays LIMIT 1", + ImmutableList.of( + newScanQueryBuilder() + .dataSource(CalciteTests.ARRAYS_DATASOURCE) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns(expressionVirtualColumn("v0", "(\"arrayLongNulls\" == array(null,null))", ColumnType.LONG)) + .columns("v0") + .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .limit(1) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + new Object[]{false} + ) + ); + } }