From b02b2c50ec3cd14d12cc7ad7d11a1aa59b0dd433 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 26 Aug 2026 05:38:11 +0300 Subject: [PATCH 1/7] fix(isthmus): build a character literal from the type its conversion produced LiteralConverter.convert derives the Substrait type twice: once through typeConverter.toSubstrait, which consults the UserTypeMapper, and again in the switch over the Calcite type name, which does not. The second derivation is the one the literal gets, so a mapped character column ended up with a schema of the mapped type and literals of the unmapped one, and VirtualTableScan rejected the relation. A null literal took its type from the first derivation and so already agreed, which is why only rows carrying a value diverged. The character branches now build from the type already derived, covering the three forms a Calcite character type can map to. A fixedchar literal carries no length of its own -- FixedCharLiteral derives it from the text -- so the text is padded to the declared width, which is also what CHAR(n) means. That half fixes a case needing no mapper at all: a LogicalValues row field wider than its literal, the shape #1064 was reported with, was still rejected for character types. A mapping to anything with no character literal form is reported where it happens rather than as a schema mismatch further down. Closes #1170. --- .../isthmus/expression/LiteralConverter.java | 49 ++++- .../isthmus/UserTypeMapperLiteralTest.java | 192 ++++++++++++++++++ 2 files changed, 232 insertions(+), 9 deletions(-) create mode 100644 isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java 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 2b8d09676..3844c1b38 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -78,6 +78,44 @@ private static String s(RexLiteral literal) { return ((NlsString) literal.getValue()).getValue(); } + /** + * Builds a character literal of the Substrait type the containing conversion already produced, + * rather than deriving the form a second time from the Calcite type. A {@link + * io.substrait.isthmus.UserTypeMapper} can map a Calcite character type to any of the three, and + * only the first derivation consults it. + * + * @param type the Substrait type this literal must carry + * @param nullable whether the literal is nullable + * @param value the literal's text + * @return the literal + */ + private static Expression.Literal characterLiteral(Type type, boolean nullable, String value) { + if (type instanceof Type.Str) { + return ExpressionCreator.string(nullable, value); + } + if (type instanceof Type.VarChar) { + return ExpressionCreator.varChar(nullable, value, ((Type.VarChar) type).length()); + } + if (type instanceof Type.FixedChar) { + // A fixedchar literal carries no length of its own — Expression.FixedCharLiteral derives it + // from the text — so the text has to be the declared width or the two disagree. Padding is + // also what CHAR(n) means: 'a' in a CHAR(3) is 'a '. + int length = ((Type.FixedChar) type).length(); + if (value.length() > length) { + throw new IllegalArgumentException( + String.format( + "Character value '%s' is longer than the fixedchar<%d> it is declared as", + value, length)); + } + return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - value.length())); + } + throw new UnsupportedOperationException( + String.format( + "A Calcite character type converted to %s, which has no character literal form; a " + + "UserTypeMapper returning it cannot be applied to the literal '%s'", + type, value)); + } + private static BigDecimal bd(RexLiteral literal) { return (BigDecimal) literal.getValue(); } @@ -134,8 +172,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { { Comparable val = literal.getValue(); if (val instanceof NlsString) { - NlsString nls = (NlsString) val; - return ExpressionCreator.fixedChar(nullable, nls.getValue()); + return characterLiteral(type, nullable, ((NlsString) val).getValue()); } throw new UnsupportedOperationException("Unable to handle char type: " + val); } @@ -152,13 +189,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { nullable, bd, resultType.getPrecision(), resultType.getScale()); } case VARCHAR: - { - if (resultType.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED) { - return ExpressionCreator.string(nullable, s(literal)); - } - - return ExpressionCreator.varChar(nullable, s(literal), resultType.getPrecision()); - } + return characterLiteral(type, nullable, s(literal)); case BINARY: return ExpressionCreator.fixedBinary( nullable, diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java new file mode 100644 index 000000000..58c069de2 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -0,0 +1,192 @@ +package io.substrait.isthmus; + +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 static org.junit.jupiter.api.Assertions.assertTrue; + +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.Type; +import io.substrait.type.TypeCreator; +import java.util.List; +import java.util.function.Function; +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.type.SqlTypeName; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.Test; + +/** + * A {@link UserTypeMapper} has to reach a relation's literals as well as its schema, or the two + * disagree and {@link VirtualTableScan} rejects the result. + */ +class UserTypeMapperLiteralTest extends PlanTestBase { + + /** + * Maps Calcite's character types to whatever the test asks for, leaving everything else alone. + */ + private static ConverterProvider providerMapping(Function characterTypes) { + UserTypeMapper mapper = + new UserTypeMapper() { + @Nullable + @Override + public Type toSubstrait(RelDataType relDataType) { + SqlTypeName name = relDataType.getSqlTypeName(); + if (name == SqlTypeName.CHAR || name == SqlTypeName.VARCHAR) { + return characterTypes.apply(relDataType.isNullable()); + } + return null; + } + + @Nullable + @Override + public RelDataType toCalcite(Type.UserDefined type) { + return null; + } + }; + return ConverterProvider.builder().typeConverter(new TypeConverter(mapper)).build(); + } + + private LogicalValues charValues(boolean withNull) { + RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR, 1); + RelDataType rowType = + typeFactory + .builder() + .add("c", typeFactory.createTypeWithNullability(charType, withNull)) + .build(); + RexLiteral a = builder.getRexBuilder().makeLiteral("a"); + ImmutableList> rows = + withNull + ? ImmutableList.of( + ImmutableList.of(a), + ImmutableList.of( + builder + .getRexBuilder() + .makeNullLiteral(typeFactory.createTypeWithNullability(charType, true)))) + : ImmutableList.of(ImmutableList.of(a)); + return LogicalValues.create(builder.getCluster(), rowType, rows); + } + + private VirtualTableScan convert(ConverterProvider provider, LogicalValues values) { + return assertInstanceOf(VirtualTableScan.class, SubstraitRelVisitor.convert(values, provider)); + } + + @Test + void mappedCharacterTypeReachesTheLiterals() { + VirtualTableScan converted = + convert( + providerMapping( + nullable -> nullable ? TypeCreator.NULLABLE.STRING : TypeCreator.REQUIRED.STRING), + charValues(false)); + + assertEquals(List.of(R.STRING), converted.getInitialSchema().struct().fields()); + assertEquals( + List.of(ExpressionCreator.string(false, "a")), converted.getRows().get(0).fields()); + } + + @Test + void mappedVarcharWidthReachesTheLiterals() { + VirtualTableScan converted = + convert( + providerMapping( + nullable -> + nullable ? TypeCreator.NULLABLE.varChar(40) : TypeCreator.REQUIRED.varChar(40)), + charValues(false)); + + assertEquals(List.of(R.varChar(40)), converted.getInitialSchema().struct().fields()); + assertEquals( + List.of(ExpressionCreator.varChar(false, "a", 40)), converted.getRows().get(0).fields()); + } + + /** + * The mapped type already reached a null literal, which takes its type from the conversion rather + * than being rebuilt; the non-null one beside it has to agree with it. + */ + @Test + void nullAndNonNullLiteralsCarryTheSameMappedType() { + VirtualTableScan converted = + convert( + providerMapping( + nullable -> nullable ? TypeCreator.NULLABLE.STRING : TypeCreator.REQUIRED.STRING), + charValues(true)); + + assertEquals(List.of(N.STRING), converted.getInitialSchema().struct().fields()); + assertEquals(List.of(ExpressionCreator.string(true, "a")), converted.getRows().get(0).fields()); + assertEquals( + List.of(ExpressionCreator.typedNull(N.STRING)), converted.getRows().get(1).fields()); + } + + /** + * A fixedchar literal carries no length of its own, so its text has to be padded to the declared + * width. This bites without any mapper too, whenever a row field is wider than its literal. + */ + @Test + void aFixedCharLiteralIsPaddedToTheDeclaredWidth() { + VirtualTableScan converted = + convert( + providerMapping( + nullable -> + nullable + ? TypeCreator.NULLABLE.fixedChar(40) + : TypeCreator.REQUIRED.fixedChar(40)), + charValues(false)); + + assertEquals(List.of(R.fixedChar(40)), converted.getInitialSchema().struct().fields()); + Expression only = converted.getRows().get(0).fields().get(0); + assertEquals(R.fixedChar(40), only.getType()); + assertEquals(ExpressionCreator.fixedChar(false, "a" + " ".repeat(39)), only); + } + + @Test + void aFixedCharLiteralNarrowerThanItsRowFieldIsPaddedWithoutAnyMapper() { + RelDataType wide = typeFactory.createSqlType(SqlTypeName.CHAR, 3); + RelDataType rowType = typeFactory.builder().add("c", wide).build(); + LogicalValues values = + LogicalValues.create( + builder.getCluster(), + rowType, + ImmutableList.of(ImmutableList.of(builder.getRexBuilder().makeLiteral("a")))); + + VirtualTableScan converted = convert(ConverterProvider.DEFAULT, values); + + assertEquals(List.of(R.fixedChar(3)), converted.getInitialSchema().struct().fields()); + assertEquals( + List.of(ExpressionCreator.fixedChar(false, "a ")), converted.getRows().get(0).fields()); + } + + @Test + void aCharacterValueWiderThanItsDeclaredTypeIsRejected() { + ConverterProvider provider = + providerMapping( + nullable -> + nullable ? TypeCreator.NULLABLE.fixedChar(1) : TypeCreator.REQUIRED.fixedChar(1)); + RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR, 3); + RelDataType rowType = typeFactory.builder().add("c", charType).build(); + LogicalValues values = + LogicalValues.create( + builder.getCluster(), + rowType, + ImmutableList.of(ImmutableList.of(builder.getRexBuilder().makeLiteral("abc")))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> convert(provider, values)); + assertTrue(e.getMessage().contains("fixedchar<1>"), e.getMessage()); + } + + /** A mapping with no character literal form is reported where it happens, not downstream. */ + @Test + void mappingWithNoCharacterLiteralFormIsRejected() { + ConverterProvider provider = + providerMapping(nullable -> nullable ? TypeCreator.NULLABLE.I64 : TypeCreator.REQUIRED.I64); + + UnsupportedOperationException e = + assertThrows( + UnsupportedOperationException.class, () -> convert(provider, charValues(false))); + assertTrue(e.getMessage().contains("I64"), e.getMessage()); + assertTrue(e.getMessage().contains("UserTypeMapper"), e.getMessage()); + } +} From 5f4f24d1f733d176e415dd8f8637cf90e8deb45c Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 28 Aug 2026 10:41:28 +0300 Subject: [PATCH 2/7] docs(isthmus): say why a varchar literal is not checked against its length The fixedchar branch rejects a value wider than the type it is declared as, because the literal carries no length of its own and the text is the length. A varchar literal does carry one, and a value longer than it goes unchecked -- by this conversion and by the POJO. --- .../java/io/substrait/isthmus/expression/LiteralConverter.java | 3 +++ 1 file changed, 3 insertions(+) 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 3844c1b38..7482995d9 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -94,6 +94,9 @@ private static Expression.Literal characterLiteral(Type type, boolean nullable, return ExpressionCreator.string(nullable, value); } if (type instanceof Type.VarChar) { + // Unlike a fixedchar, a varchar literal carries a length of its own, so a value shorter than + // it is what the type means. A value longer than it is malformed, and nothing rejects it -- + // here or in the POJO that holds it. return ExpressionCreator.varChar(nullable, value, ((Type.VarChar) type).length()); } if (type instanceof Type.FixedChar) { From a108ed6c22626bd2f365b582e26727311985ae92 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 16:35:44 +0300 Subject: [PATCH 3/7] fix(isthmus): keep the Calcite form for a mapping outside the character family --- .../isthmus/expression/LiteralConverter.java | 26 +++++++--- .../isthmus/UserTypeMapperLiteralTest.java | 51 ++++++++++++++++--- 2 files changed, 61 insertions(+), 16 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 7482995d9..d4bc0bc94 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -84,12 +84,24 @@ private static String s(RexLiteral literal) { * io.substrait.isthmus.UserTypeMapper} can map a Calcite character type to any of the three, and * only the first derivation consults it. * - * @param type the Substrait type this literal must carry + *

A mapping to something outside the character family keeps the form Calcite declares. Its + * type has no character literal form to build, and reaching one would need the value's encoding + * in that type, which {@link io.substrait.isthmus.UserTypeMapper} has no way to give. + * + * @param mappedType the Substrait type the containing conversion produced * @param nullable whether the literal is nullable * @param value the literal's text + * @param calciteType the Calcite type the literal was declared as * @return the literal */ - private static Expression.Literal characterLiteral(Type type, boolean nullable, String value) { + private static Expression.Literal characterLiteral( + Type mappedType, boolean nullable, String value, RelDataType calciteType) { + Type type = + mappedType instanceof Type.Str + || mappedType instanceof Type.VarChar + || mappedType instanceof Type.FixedChar + ? mappedType + : TypeConverter.DEFAULT.toSubstrait(calciteType); if (type instanceof Type.Str) { return ExpressionCreator.string(nullable, value); } @@ -112,11 +124,9 @@ private static Expression.Literal characterLiteral(Type type, boolean nullable, } return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - value.length())); } - throw new UnsupportedOperationException( + throw new IllegalStateException( String.format( - "A Calcite character type converted to %s, which has no character literal form; a " - + "UserTypeMapper returning it cannot be applied to the literal '%s'", - type, value)); + "A Calcite character type converted to %s, which is not a character type", type)); } private static BigDecimal bd(RexLiteral literal) { @@ -175,7 +185,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { { Comparable val = literal.getValue(); if (val instanceof NlsString) { - return characterLiteral(type, nullable, ((NlsString) val).getValue()); + return characterLiteral(type, nullable, ((NlsString) val).getValue(), resultType); } throw new UnsupportedOperationException("Unable to handle char type: " + val); } @@ -192,7 +202,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { nullable, bd, resultType.getPrecision(), resultType.getScale()); } case VARCHAR: - return characterLiteral(type, nullable, s(literal)); + return characterLiteral(type, nullable, s(literal), resultType); case BINARY: return ExpressionCreator.fixedBinary( nullable, diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java index 58c069de2..cae9031c4 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -8,14 +8,19 @@ import com.google.common.collect.ImmutableList; import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; +import io.substrait.relation.Project; import io.substrait.relation.VirtualTableScan; import io.substrait.type.Type; import io.substrait.type.TypeCreator; import java.util.List; +import java.util.Set; import java.util.function.Function; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.logical.LogicalProject; import org.apache.calcite.rel.logical.LogicalValues; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; import org.apache.calcite.sql.type.SqlTypeName; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; @@ -26,6 +31,8 @@ */ class UserTypeMapperLiteralTest extends PlanTestBase { + private static final String URN = "extension:test:user_types"; + /** * Maps Calcite's character types to whatever the test asks for, leaving everything else alone. */ @@ -177,16 +184,44 @@ void aCharacterValueWiderThanItsDeclaredTypeIsRejected() { assertTrue(e.getMessage().contains("fixedchar<1>"), e.getMessage()); } - /** A mapping with no character literal form is reported where it happens, not downstream. */ + /** A projection of a character literal, which no schema stands behind. */ + private RelNode charLiteralProject() { + RelNode input = builder.values(new String[] {"i"}, 1).build(); + RexNode literal = builder.getRexBuilder().makeLiteral("a"); + return LogicalProject.create(input, List.of(), List.of(literal), List.of("c"), Set.of()); + } + + /** + * A mapping outside the character family leaves the literal in the form Calcite declares. Its + * type has no character literal form to build, and reaching one would need the value's encoding + * in that type, which a {@link UserTypeMapper} has no way to give. + */ @Test - void mappingWithNoCharacterLiteralFormIsRejected() { + void aMappingWithNoCharacterLiteralFormKeepsTheCalciteForm() { ConverterProvider provider = - providerMapping(nullable -> nullable ? TypeCreator.NULLABLE.I64 : TypeCreator.REQUIRED.I64); + providerMapping(nullable -> TypeCreator.of(nullable).userDefined(URN, "u_type")); + + Project project = + assertInstanceOf( + Project.class, SubstraitRelVisitor.convert(charLiteralProject(), provider)); + + assertEquals(ExpressionCreator.fixedChar(false, "a"), project.getExpressions().get(0)); + } + + /** + * What that leaves open: a virtual table's rows still have to carry the schema's types, and a + * mapping outside the character family puts the two out of step -- the schema takes the mapped + * type while the literal keeps Calcite's. Closing it needs a literal-side hook the mapper does + * not have. + */ + @Test + void aMappingWithNoCharacterLiteralFormStillDisagreesWithAVirtualTableSchema() { + ConverterProvider provider = + providerMapping(nullable -> TypeCreator.of(nullable).userDefined(URN, "u_type")); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> convert(provider, charValues(false))); - UnsupportedOperationException e = - assertThrows( - UnsupportedOperationException.class, () -> convert(provider, charValues(false))); - assertTrue(e.getMessage().contains("I64"), e.getMessage()); - assertTrue(e.getMessage().contains("UserTypeMapper"), e.getMessage()); + assertTrue(e.getMessage().contains("does not match schema field type"), e.getMessage()); } } From 5a38540e0a82dcd54309967cc4b3bf6710f86230 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 17:27:12 +0300 Subject: [PATCH 4/7] fix(isthmus): count a fixedchar's width in characters, and give a reflective char one --- .../io/substrait/isthmus/TypeConverter.java | 10 ++- .../isthmus/expression/LiteralConverter.java | 23 ++++--- .../substrait/isthmus/CalciteLiteralTest.java | 25 ++++++++ .../io/substrait/isthmus/CalciteTypeTest.java | 18 ++++++ .../isthmus/UserTypeMapperLiteralTest.java | 62 +++++++++++++++---- 5 files changed, 118 insertions(+), 20 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index 6ebbfe3ca..754387694 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -150,7 +150,15 @@ private Type toSubstrait(RelDataType type, List names) { return creator.decimal(type.getPrecision(), type.getScale()); } case CHAR: - return creator.fixedChar(type.getPrecision()); + { + // A char or Character JavaType carries no precision of its own, which Calcite reads as + // its default of 1. Without this a reflective schema derives fixedchar<-1>, a width + // outside the [1..2147483647] the spec allows. + if (type.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED) { + return creator.fixedChar(1); + } + return creator.fixedChar(type.getPrecision()); + } case VARCHAR: { if (type.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED) { 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 d4bc0bc94..b2127cc2c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -17,6 +17,7 @@ import java.time.format.DateTimeFormatterBuilder; import java.time.temporal.ChronoField; import java.util.List; +import java.util.Locale; import java.util.Objects; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -88,14 +89,17 @@ private static String s(RexLiteral literal) { * type has no character literal form to build, and reaching one would need the value's encoding * in that type, which {@link io.substrait.isthmus.UserTypeMapper} has no way to give. * + *

Whether the literal is nullable comes from the mapped type either way, which is what the + * containing conversion took it from before the type reached here. + * * @param mappedType the Substrait type the containing conversion produced - * @param nullable whether the literal is nullable * @param value the literal's text * @param calciteType the Calcite type the literal was declared as * @return the literal */ private static Expression.Literal characterLiteral( - Type mappedType, boolean nullable, String value, RelDataType calciteType) { + Type mappedType, String value, RelDataType calciteType) { + boolean nullable = mappedType.nullable(); Type type = mappedType instanceof Type.Str || mappedType instanceof Type.VarChar @@ -115,14 +119,19 @@ private static Expression.Literal characterLiteral( // A fixedchar literal carries no length of its own — Expression.FixedCharLiteral derives it // from the text — so the text has to be the declared width or the two disagree. Padding is // also what CHAR(n) means: 'a' in a CHAR(3) is 'a '. + // In characters rather than UTF-16 code units: the spec gives a fixedchar its length in + // characters, where it spells a string's out in UTF-8 bytes. int length = ((Type.FixedChar) type).length(); - if (value.length() > length) { + int characters = value.codePointCount(0, value.length()); + if (characters > length) { throw new IllegalArgumentException( String.format( + Locale.ROOT, "Character value '%s' is longer than the fixedchar<%d> it is declared as", - value, length)); + value, + length)); } - return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - value.length())); + return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - characters)); } throw new IllegalStateException( String.format( @@ -185,7 +194,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { { Comparable val = literal.getValue(); if (val instanceof NlsString) { - return characterLiteral(type, nullable, ((NlsString) val).getValue(), resultType); + return characterLiteral(type, ((NlsString) val).getValue(), resultType); } throw new UnsupportedOperationException("Unable to handle char type: " + val); } @@ -202,7 +211,7 @@ public Expression.Literal convert(RexLiteral literal, RelDataType resultType) { nullable, bd, resultType.getPrecision(), resultType.getScale()); } case VARCHAR: - return characterLiteral(type, nullable, s(literal), resultType); + return characterLiteral(type, s(literal), resultType); case BINARY: return ExpressionCreator.fixedBinary( nullable, diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java index 6952759b3..a2db08544 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java @@ -645,6 +645,31 @@ void tStruct() { false)); } + /** A varchar literal carries the width its column declares, not the length of its text. */ + @Test + void tVarCharCarriesTheDeclaredWidth() { + RexLiteral literal = (RexLiteral) rex.makeLiteral("a"); + + assertEquals( + ExpressionCreator.varChar(false, "a", 10), + new LiteralConverter(TypeConverter.DEFAULT).convert(literal, t(SqlTypeName.VARCHAR, 10))); + } + + /** + * A VARCHAR with no declared width is a Substrait {@code string}, which carries none. A + * reflective schema's String column is where one comes from. + */ + @Test + void tVarCharWithoutADeclaredWidthIsAString() { + RexLiteral literal = (RexLiteral) rex.makeLiteral("a"); + RelDataType unspecified = + ((org.apache.calcite.adapter.java.JavaTypeFactory) type).createJavaType(String.class); + + assertEquals( + ExpressionCreator.string(true, "a"), + new LiteralConverter(TypeConverter.DEFAULT).convert(literal, unspecified)); + } + @Test void tStructUsesResultFieldTypes() { RelDataType literalType = type.createStructType(List.of(t(SqlTypeName.TINYINT)), List.of("c1")); diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java index b95725b32..50199128f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java @@ -202,6 +202,24 @@ void varchar(boolean nullable) { testType(Type.withNullability(nullable).varChar(74), SqlTypeName.VARCHAR, nullable, 74); } + /** + * A char or Character column of a reflective schema carries no width of its own, which Calcite + * reads as its default of 1. A fixedchar of the unspecified width would be a {@code + * fixedchar<-1>}, outside the [1..2147483647] the spec allows. + */ + @Test + void aJavaCharColumnTakesCalcitesDefaultWidth() { + org.apache.calcite.adapter.java.JavaTypeFactory javaTypeFactory = + (org.apache.calcite.adapter.java.JavaTypeFactory) type; + + assertEquals( + TypeCreator.REQUIRED.fixedChar(1), + TypeConverter.DEFAULT.toSubstrait(javaTypeFactory.createJavaType(char.class))); + assertEquals( + TypeCreator.NULLABLE.fixedChar(1), + TypeConverter.DEFAULT.toSubstrait(javaTypeFactory.createJavaType(Character.class))); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void decimal(boolean nullable) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java index cae9031c4..2312df487 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -1,5 +1,6 @@ package io.substrait.isthmus; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -8,6 +9,7 @@ import com.google.common.collect.ImmutableList; import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; +import io.substrait.isthmus.expression.LiteralConverter; import io.substrait.relation.Project; import io.substrait.relation.VirtualTableScan; import io.substrait.type.Type; @@ -21,7 +23,10 @@ import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlCollation; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.calcite.util.ConversionUtil; +import org.apache.calcite.util.NlsString; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; @@ -37,6 +42,11 @@ class UserTypeMapperLiteralTest extends PlanTestBase { * Maps Calcite's character types to whatever the test asks for, leaving everything else alone. */ private static ConverterProvider providerMapping(Function characterTypes) { + return ConverterProvider.builder().typeConverter(typeConverterMapping(characterTypes)).build(); + } + + /** The same mapping, for a test that converts a literal rather than a relation. */ + private static TypeConverter typeConverterMapping(Function characterTypes) { UserTypeMapper mapper = new UserTypeMapper() { @Nullable @@ -55,7 +65,7 @@ public RelDataType toCalcite(Type.UserDefined type) { return null; } }; - return ConverterProvider.builder().typeConverter(new TypeConverter(mapper)).build(); + return new TypeConverter(mapper); } private LogicalValues charValues(boolean withNull) { @@ -165,25 +175,53 @@ void aFixedCharLiteralNarrowerThanItsRowFieldIsPaddedWithoutAnyMapper() { List.of(ExpressionCreator.fixedChar(false, "a ")), converted.getRows().get(0).fields()); } + /** + * Asserted on the conversion of the literal rather than of a relation holding it: {@link + * VirtualTableScan} rejects a row disagreeing with its schema with the same exception type, so a + * relation-level assertion passes whether this guard is there or not. + */ @Test void aCharacterValueWiderThanItsDeclaredTypeIsRejected() { - ConverterProvider provider = - providerMapping( - nullable -> - nullable ? TypeCreator.NULLABLE.fixedChar(1) : TypeCreator.REQUIRED.fixedChar(1)); + LiteralConverter converter = + new LiteralConverter( + typeConverterMapping(nullable -> TypeCreator.of(nullable).fixedChar(1))); + RexLiteral literal = (RexLiteral) builder.getRexBuilder().makeLiteral("abc"); RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR, 3); - RelDataType rowType = typeFactory.builder().add("c", charType).build(); - LogicalValues values = - LogicalValues.create( - builder.getCluster(), - rowType, - ImmutableList.of(ImmutableList.of(builder.getRexBuilder().makeLiteral("abc")))); IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> convert(provider, values)); + assertThrows(IllegalArgumentException.class, () -> converter.convert(literal, charType)); assertTrue(e.getMessage().contains("fixedchar<1>"), e.getMessage()); } + /** + * A fixedchar's width is a count of characters -- the spec gives a string's length in UTF-8 bytes + * and a fixedchar's in characters -- so one astral character fills a {@code fixedchar<1>} and + * leaves two spaces to pad in a {@code fixedchar<3>}. + */ + @Test + void aFixedCharWidthCountsCharactersRatherThanCodeUnits() { + LiteralConverter converter = new LiteralConverter(TypeConverter.DEFAULT); + // Calcite's default charset is ISO-8859-1, which cannot hold the character at all. + RexLiteral clef = + builder + .getRexBuilder() + .makeCharLiteral( + new NlsString( + "\uD834\uDD1E", + ConversionUtil.NATIVE_UTF16_CHARSET_NAME, + SqlCollation.IMPLICIT)); + + assertAll( + () -> + assertEquals( + ExpressionCreator.fixedChar(false, "\uD834\uDD1E"), + converter.convert(clef, typeFactory.createSqlType(SqlTypeName.CHAR, 1))), + () -> + assertEquals( + ExpressionCreator.fixedChar(false, "\uD834\uDD1E "), + converter.convert(clef, typeFactory.createSqlType(SqlTypeName.CHAR, 3)))); + } + /** A projection of a character literal, which no schema stands behind. */ private RelNode charLiteralProject() { RelNode input = builder.values(new String[] {"i"}, 1).build(); From b569ed7e6ee788b4e9446b58e692b631df2d1d81 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 18:03:21 +0300 Subject: [PATCH 5/7] fix(isthmus): refuse a fixedchar width no Java String can hold --- .../isthmus/expression/LiteralConverter.java | 24 +++++++++++++++++++ .../isthmus/UserTypeMapperLiteralTest.java | 19 +++++++++++++++ 2 files changed, 43 insertions(+) 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 b2127cc2c..bab324112 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -60,6 +60,19 @@ public class LiteralConverter { .append(CALCITE_LOCAL_TIME_FORMATTER) .toFormatter(); + /** + * The longest text a {@code fixedchar} literal can be padded to. + * + *

A Java {@link String} holds its characters in an array, and this is the largest one a JVM + * allocates, so a wider literal cannot be built whatever the heap. {@code String.repeat} reports + * that as an {@link OutOfMemoryError}, which no {@code catch (Exception)} sees and which names + * neither the column nor the value. A width the heap alone cannot hold still fails as one. + * + *

The width a fixedchar may declare is the spec's {@code [1..2147483647]}; what a plan's + * target engine actually supports is a dialect's answer, not this. + */ + private static final int MAX_PADDED_LENGTH = Integer.MAX_VALUE - 8; + private final TypeConverter typeConverter; /** @@ -131,6 +144,17 @@ private static Expression.Literal characterLiteral( value, length)); } + long padded = (long) value.length() + ((long) length - characters); + if (padded > MAX_PADDED_LENGTH) { + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "A fixedchar<%d> literal cannot be built from '%s': padding it to that width takes " + + "%d characters, more than a Java String holds", + length, + value, + padded)); + } return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - characters)); } throw new IllegalStateException( diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java index 2312df487..f76e8afcf 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -222,6 +222,25 @@ void aFixedCharWidthCountsCharactersRatherThanCodeUnits() { converter.convert(clef, typeFactory.createSqlType(SqlTypeName.CHAR, 3)))); } + /** + * A mapper hands the literal its type directly, so no type-system ceiling stands between a width + * and the padding it asks for. A width no Java String can hold is refused where the padding + * happens, rather than reaching {@code String.repeat} as an {@link OutOfMemoryError}. + */ + @Test + void aWidthNoStringCanHoldIsRefusedRatherThanPadded() { + LiteralConverter converter = + new LiteralConverter( + typeConverterMapping( + nullable -> TypeCreator.of(nullable).fixedChar(Integer.MAX_VALUE))); + RexLiteral literal = (RexLiteral) builder.getRexBuilder().makeLiteral("a"); + RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR, 1); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> converter.convert(literal, charType)); + assertTrue(e.getMessage().contains("more than a Java String holds"), e.getMessage()); + } + /** A projection of a character literal, which no schema stands behind. */ private RelNode charLiteralProject() { RelNode input = builder.values(new String[] {"i"}, 1).build(); From 9f3649e01bb3c3cace3032e46ed0a51f922e2130 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 22:46:45 +0300 Subject: [PATCH 6/7] fix(isthmus): refuse a negative fixedchar width A UserTypeMapper answers with a Substrait type directly, so nothing between it and the padding here holds its width to what a fixedchar can declare. A negative width did throw, but reported the value as longer than the fixedchar<-5> it is declared as, which is not the problem. The floor stops at the negatives. type_classes.md puts a fixedchar's width in [1..2147483647] (spec v0.101.0), but Calcite types '' as a CHAR(0) and its DDL parser takes a CHAR(0) column, so refusing a zero width here would stop ordinary SQL converting. The padded-width message says UTF-16 code units, which is what it counts: a value with an astral character makes those differ from the characters the width check above it counts. --- .../isthmus/expression/LiteralConverter.java | 12 +++++++++++- .../isthmus/UserTypeMapperLiteralTest.java | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) 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 bab324112..de3184e42 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -135,6 +135,16 @@ private static Expression.Literal characterLiteral( // In characters rather than UTF-16 code units: the spec gives a fixedchar its length in // characters, where it spells a string's out in UTF-8 bytes. int length = ((Type.FixedChar) type).length(); + // Only the negative end. The spec puts a fixedchar's width in [1..2147483647] (spec + // v0.101.0), but Calcite types the empty character literal as a CHAR(0) and its DDL parser + // takes a CHAR(0) column, so refusing a zero width here would stop ordinary SQL converting. + if (length < 0) { + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "A fixedchar cannot declare a negative width, and this one is %d", + length)); + } int characters = value.codePointCount(0, value.length()); if (characters > length) { throw new IllegalArgumentException( @@ -150,7 +160,7 @@ private static Expression.Literal characterLiteral( String.format( Locale.ROOT, "A fixedchar<%d> literal cannot be built from '%s': padding it to that width takes " - + "%d characters, more than a Java String holds", + + "%d UTF-16 code units, more than a Java String holds", length, value, padded)); diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java index f76e8afcf..3c4acd856 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -241,6 +241,25 @@ void aWidthNoStringCanHoldIsRefusedRatherThanPadded() { assertTrue(e.getMessage().contains("more than a Java String holds"), e.getMessage()); } + /** + * A mapper answers with a Substrait type directly, so nothing between it and the padding here + * holds its width to what a fixedchar can declare. A zero width is left alone: the spec puts the + * range at [1..2147483647], but ordinary SQL produces a CHAR(0) today. + */ + @Test + void aNegativeFixedCharWidthIsRefused() { + RexLiteral literal = (RexLiteral) builder.getRexBuilder().makeLiteral("a"); + RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR, 1); + LiteralConverter negative = + new LiteralConverter( + typeConverterMapping(nullable -> TypeCreator.of(nullable).fixedChar(-5))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> negative.convert(literal, charType)); + + assertTrue(e.getMessage().contains("negative width, and this one is -5"), e.getMessage()); + } + /** A projection of a character literal, which no schema stands behind. */ private RelNode charLiteralProject() { RelNode input = builder.values(new String[] {"i"}, 1).build(); From 830c0b6331086489b75f4722bf7f493d2e06835f Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 13:29:31 +0300 Subject: [PATCH 7/7] docs(isthmus): drop the spec marker from a bound that is not new The [1..2147483647] a fixedchar's width sits in goes back to spec #200 in 2022, so a version marker beside it reads as provenance it does not have and would need editing on every unrelated bump. The javadoc above and TypeConverter's CHAR comment already cite the same range without one. --- .../io/substrait/isthmus/expression/LiteralConverter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 de3184e42..ebd14d978 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java @@ -135,9 +135,9 @@ private static Expression.Literal characterLiteral( // In characters rather than UTF-16 code units: the spec gives a fixedchar its length in // characters, where it spells a string's out in UTF-8 bytes. int length = ((Type.FixedChar) type).length(); - // Only the negative end. The spec puts a fixedchar's width in [1..2147483647] (spec - // v0.101.0), but Calcite types the empty character literal as a CHAR(0) and its DDL parser - // takes a CHAR(0) column, so refusing a zero width here would stop ordinary SQL converting. + // Only the negative end. The spec puts a fixedchar's width in [1..2147483647], but Calcite + // types the empty character literal as a CHAR(0) and its DDL parser takes a CHAR(0) column, + // so refusing a zero width here would stop ordinary SQL converting. if (length < 0) { throw new IllegalArgumentException( String.format(