diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 68c1d2b50..a20fd45e7 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -163,19 +163,19 @@ public Rel visit(org.apache.calcite.rel.core.Values values) { NamedStruct type = typeConverter.toNamedStruct(values.getRowType()); LiteralConverter literalConverter = new LiteralConverter(typeConverter); - List schemaFieldTypes = type.struct().fields(); List structs = values.getTuples().stream() .map( list -> { - // Use schema nullability since Calcite infers non-nullable for all non-null - // values + // Calcite may infer a narrower type for a tuple literal than for its row field. + // Virtual table rows must use the complete schema type. List fields = IntStream.range(0, list.size()) .mapToObj( i -> literalConverter.convert( - list.get(i), schemaFieldTypes.get(i).nullable())) + list.get(i), + values.getRowType().getFieldList().get(i).getType())) .collect(Collectors.toUnmodifiableList()); return ExpressionCreator.nestedStruct(false, fields); }) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java index 2cf26e02f..d42894855 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -21,6 +21,7 @@ import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.util.DateString; @@ -91,31 +92,33 @@ private static BigDecimal bd(RexLiteral literal) { * @throws UnsupportedOperationException if the literal type/value cannot be handled */ public Expression.Literal convert(RexLiteral literal) { - // convert type first to guarantee we can handle the value. - final Type type = typeConverter.toSubstrait(literal.getType()); - return convert(literal, type.nullable()); + return convert(literal, literal.getType()); } /** - * Converts a RexLiteral to a Substrait Literal with the specified nullability. + * Converts a RexLiteral to a Substrait Literal carrying the given result type. * - *

This overload is useful when the target nullability should come from the schema rather than - * the literal's own type. For example, Calcite's LogicalValues may have literals with - * non-nullable types even when the schema field is nullable. + *

This overload is useful when the target type comes from a containing schema rather than the + * literal itself. Calcite may infer a narrower type for a value in a LogicalValues tuple than for + * the corresponding row field. Nullability is taken from {@code resultType}, so callers that need + * a nullability other than the literal's own should widen the Calcite type with {@link + * org.apache.calcite.rel.type.RelDataTypeFactory#createTypeWithNullability} before calling. * * @param literal the RexLiteral to convert - * @param nullable the nullability to use for the resulting Substrait literal + * @param resultType the Calcite type required by the containing schema * @return the converted Substrait Literal */ - public Expression.Literal convert(RexLiteral literal, boolean nullable) { + public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { + // convert type first to guarantee we can handle the value. + final Type type = typeConverter.toSubstrait(resultType); + final boolean nullable = type.nullable(); if (literal.isNull()) { - final Type type = typeConverter.toSubstrait(literal.getType()); final Type typeWithNullability = nullable ? TypeCreator.asNullable(type) : TypeCreator.asNotNullable(type); return ExpressionCreator.typedNull(typeWithNullability); } - switch (literal.getType().getSqlTypeName()) { + switch (resultType.getSqlTypeName()) { case TINYINT: return ExpressionCreator.i8(nullable, i(literal).intValue()); case SMALLINT: @@ -145,15 +148,15 @@ public Expression.Literal convert(RexLiteral literal, boolean nullable) { { BigDecimal bd = bd(literal); return ExpressionCreator.decimal( - nullable, bd, literal.getType().getPrecision(), literal.getType().getScale()); + nullable, bd, resultType.getPrecision(), resultType.getScale()); } case VARCHAR: { - if (literal.getType().getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED) { + if (resultType.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED) { return ExpressionCreator.string(nullable, s(literal)); } - return ExpressionCreator.varChar(nullable, s(literal), literal.getType().getPrecision()); + return ExpressionCreator.varChar(nullable, s(literal), resultType.getPrecision()); } case BINARY: return ExpressionCreator.fixedBinary( @@ -161,7 +164,7 @@ public Expression.Literal convert(RexLiteral literal, boolean nullable) { ByteString.copyFrom( padRightIfNeeded( literal.getValueAs(org.apache.calcite.avatica.util.ByteString.class), - literal.getType().getPrecision()))); + resultType.getPrecision()))); case VARBINARY: return ExpressionCreator.binary( nullable, ByteString.copyFrom(literal.getValueAs(byte[].class))); @@ -246,21 +249,29 @@ public Expression.Literal convert(RexLiteral literal, boolean nullable) { { List literals = (List) literal.getValue(); return ExpressionCreator.struct( - nullable, literals.stream().map(this::convert).collect(Collectors.toList())); + nullable, + IntStream.range(0, literals.size()) + .mapToObj( + i -> convert(literals.get(i), resultType.getFieldList().get(i).getType())) + .collect(Collectors.toList())); } case ARRAY: { List literals = (List) literal.getValue(); + RelDataType componentType = Objects.requireNonNull(resultType.getComponentType()); return ExpressionCreator.list( - nullable, literals.stream().map(this::convert).collect(Collectors.toList())); + nullable, + literals.stream() + .map(nestedLiteral -> convert(nestedLiteral, componentType)) + .collect(Collectors.toList())); } default: throw new UnsupportedOperationException( String.format( "Unable to convert the value of %s of type %s to a literal.", - literal, literal.getType().getSqlTypeName())); + literal, resultType.getSqlTypeName())); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index e038cedd6..0b177707f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -2,14 +2,17 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; +import com.google.common.collect.ImmutableList; import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; import io.substrait.relation.VirtualTableScan; import io.substrait.type.NamedStruct; import java.io.PrintWriter; import java.io.StringWriter; +import java.math.BigDecimal; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -17,7 +20,11 @@ import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.RelWriter; import org.apache.calcite.rel.externalize.RelWriterImpl; +import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.sql.SqlExplainLevel; +import org.apache.calcite.sql.type.SqlTypeName; import org.junit.jupiter.api.Test; class VirtualTableScanTest extends PlanTestBase { @@ -111,6 +118,26 @@ void mixedNullabilityRoundTrip() { assertFullRoundTrip(virtualTableScan); } + @Test + void valuesLiteralUsesSchemaType() { + RelDataType rowType = typeFactory.builder().add("col1", SqlTypeName.INTEGER).build(); + RexLiteral literal = + builder + .getRexBuilder() + .makeExactLiteral(BigDecimal.ONE, typeFactory.createSqlType(SqlTypeName.TINYINT)); + LogicalValues values = + LogicalValues.create( + builder.getCluster(), rowType, ImmutableList.of(ImmutableList.of(literal))); + + VirtualTableScan converted = + assertInstanceOf( + VirtualTableScan.class, SubstraitRelVisitor.convert(values, converterProvider)); + assertEquals(R.I32, converted.getInitialSchema().struct().fields().get(0)); + Expression.I32Literal convertedLiteral = + assertInstanceOf(Expression.I32Literal.class, converted.getRows().get(0).fields().get(0)); + assertEquals(1, convertedLiteral.value()); + } + @SafeVarargs private VirtualTableScan createVirtualTableScan(NamedStruct schema, List... rows) { List structs =