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 2b8d09676..ebd14d978 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; @@ -59,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; /** @@ -78,6 +92,86 @@ 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. + * + *

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. + * + *

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 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, String value, RelDataType calciteType) { + boolean nullable = mappedType.nullable(); + 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); + } + 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) { + // 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(); + // 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( + 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( + String.format( + Locale.ROOT, + "Character value '%s' is longer than the fixedchar<%d> it is declared as", + 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 UTF-16 code units, more than a Java String holds", + length, + value, + padded)); + } + return ExpressionCreator.fixedChar(nullable, value + " ".repeat(length - characters)); + } + throw new IllegalStateException( + String.format( + "A Calcite character type converted to %s, which is not a character type", type)); + } + private static BigDecimal bd(RexLiteral literal) { return (BigDecimal) literal.getValue(); } @@ -134,8 +228,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, ((NlsString) val).getValue(), resultType); } throw new UnsupportedOperationException("Unable to handle char type: " + val); } @@ -152,13 +245,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, 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 new file mode 100644 index 000000000..3c4acd856 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/UserTypeMapperLiteralTest.java @@ -0,0 +1,303 @@ +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; +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.isthmus.expression.LiteralConverter; +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.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; + +/** + * 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 { + + private static final String URN = "extension:test:user_types"; + + /** + * 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 + @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 new TypeConverter(mapper); + } + + 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()); + } + + /** + * 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() { + 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); + + IllegalArgumentException e = + 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 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 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(); + 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 aMappingWithNoCharacterLiteralFormKeepsTheCalciteForm() { + ConverterProvider provider = + 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))); + + assertTrue(e.getMessage().contains("does not match schema field type"), e.getMessage()); + } +}