From 0ce6102138802b72026420b24adfa78bade2bd15 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 5 Aug 2026 15:43:43 +0300 Subject: [PATCH 1/2] fix(isthmus): use schema types for virtual table literals Use the complete LogicalValues row-schema field type when converting tuple literals so virtual table rows match their schema. Preserve literal-derived behavior for other conversions and cover TINYINT-to-INTEGER widening. Fixes #1064 --- .../isthmus/SubstraitRelVisitor.java | 8 +-- .../isthmus/expression/LiteralConverter.java | 51 ++++++++++++++----- .../isthmus/VirtualTableScanTest.java | 27 ++++++++++ 3 files changed, 70 insertions(+), 16 deletions(-) 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..4fd387e44 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,9 +92,7 @@ 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()); } /** @@ -108,14 +107,34 @@ public Expression.Literal convert(RexLiteral literal) { * @return the converted Substrait Literal */ public Expression.Literal convert(RexLiteral literal, boolean nullable) { + return convert(literal, literal.getType(), nullable); + } + + /** + * Converts a RexLiteral to a Substrait Literal using the specified result type. + * + *

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. + * + * @param literal the RexLiteral to convert + * @param resultType the Calcite type required by the containing schema + * @return the converted Substrait Literal + */ + public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { + Type type = typeConverter.toSubstrait(resultType); + return convert(literal, resultType, type.nullable()); + } + + private Expression.Literal convert(RexLiteral literal, RelDataType resultType, boolean nullable) { if (literal.isNull()) { - final Type type = typeConverter.toSubstrait(literal.getType()); + final Type type = typeConverter.toSubstrait(resultType); 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 +164,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 +180,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 +265,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 = From 697ad4e8b4a4049b4dada95297ca1e02f1b8ee2e Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 7 Aug 2026 10:44:07 +0300 Subject: [PATCH 2/2] refactor(isthmus)!: drop the nullability-only LiteralConverter overload convert(RexLiteral, boolean) has no callers left now that virtual table literals carry the schema type, and its nullability argument duplicates what the Calcite result type already expresses. BREAKING CHANGE: io.substrait.isthmus.expression.LiteralConverter#convert(RexLiteral, boolean) is removed. Use convert(RexLiteral, RelDataType), which takes nullability from the supplied Calcite type; callers needing a nullability other than the literal's own should widen the type with RelDataTypeFactory#createTypeWithNullability first. --- .../isthmus/expression/LiteralConverter.java | 30 +++++-------------- 1 file changed, 7 insertions(+), 23 deletions(-) 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 4fd387e44..d42894855 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -96,39 +96,23 @@ public Expression.Literal convert(RexLiteral literal) { } /** - * Converts a RexLiteral to a Substrait Literal with the specified nullability. - * - *

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. - * - * @param literal the RexLiteral to convert - * @param nullable the nullability to use for the resulting Substrait literal - * @return the converted Substrait Literal - */ - public Expression.Literal convert(RexLiteral literal, boolean nullable) { - return convert(literal, literal.getType(), nullable); - } - - /** - * Converts a RexLiteral to a Substrait Literal using the specified result type. + * Converts a RexLiteral to a Substrait Literal carrying the given result type. * *

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. + * 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 resultType the Calcite type required by the containing schema * @return the converted Substrait Literal */ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { - Type type = typeConverter.toSubstrait(resultType); - return convert(literal, resultType, type.nullable()); - } - - private Expression.Literal convert(RexLiteral literal, RelDataType resultType, boolean nullable) { + // 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(resultType); final Type typeWithNullability = nullable ? TypeCreator.asNullable(type) : TypeCreator.asNotNullable(type); return ExpressionCreator.typedNull(typeWithNullability);