Skip to content

Commit

Permalink
fix bug with CastOperatorConversion with types which cannot be mapped…
Browse files Browse the repository at this point in the history
… to native druid types (#17011)
  • Loading branch information
clintropolis authored Sep 7, 2024
1 parent 48a758e commit b0f36c1
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,7 @@ public static ExpressionType operator(@Nullable ExpressionType type, @Nullable E
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) {
Expand Down Expand Up @@ -134,12 +131,8 @@ public static ExpressionType function(@Nullable ExpressionType type, @Nullable E
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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -48,9 +45,6 @@ public class OutputTypeTest extends InitializedNullHandlingTest
.build()
);

@Rule
public ExpectedException expectedException = ExpectedException.none();

@Test
public void testConstantsAndIdentifiers()
{
Expand Down Expand Up @@ -541,7 +535,6 @@ public void testOperatorAutoConversion()
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)
Expand All @@ -554,7 +547,30 @@ public void testOperatorAutoConversion()
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,
Expand Down Expand Up @@ -619,7 +635,6 @@ public void testFunctionAutoConversion()
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)
Expand All @@ -632,6 +647,30 @@ public void testFunctionAutoConversion()
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,
Expand Down Expand Up @@ -719,27 +758,6 @@ public void testIntegerFunctionAutoConversion()
);
}

@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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,17 @@ public DruidExpression toDruidExpression(
: 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ public static ColumnType getValueTypeForRelDataTypeFull(final RelDataType type)
if (elementType != null) {
return ColumnType.ofArray(elementType);
}
if (type.getComponentType().getSqlTypeName() == SqlTypeName.NULL) {
return ColumnType.LONG_ARRAY;
}
return null;
} else {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ public static void assertDeepEquals(String path, Object expected, Object actual)
}
}

// 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()
{
Expand Down Expand Up @@ -7478,4 +7476,26 @@ public void testGroupByNestedArrayInlineCount()
)
);
}

@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}
)
);
}
}

0 comments on commit b0f36c1

Please sign in to comment.