diff --git a/core/src/main/java/io/substrait/expression/Expression.java b/core/src/main/java/io/substrait/expression/Expression.java index cc77902dc..54425f22f 100644 --- a/core/src/main/java/io/substrait/expression/Expression.java +++ b/core/src/main/java/io/substrait/expression/Expression.java @@ -823,6 +823,23 @@ abstract class VarCharLiteral implements Literal { */ public abstract int length(); + /** + * Validates that the value fits within the declared maximum length in Unicode code points. + * + * @throws IllegalArgumentException if the value exceeds the declared length + */ + @Value.Check + protected void check() { + int characters = value().codePointCount(0, value().length()); + if (characters > length()) { + throw new IllegalArgumentException( + "VarCharLiteral value has " + + characters + + " characters, exceeding declared length " + + length()); + } + } + @Override public Type getType() { return Type.withNullability(nullable()).varChar(length()); diff --git a/core/src/test/java/io/substrait/expression/VarCharLiteralTest.java b/core/src/test/java/io/substrait/expression/VarCharLiteralTest.java new file mode 100644 index 000000000..52dd51fc8 --- /dev/null +++ b/core/src/test/java/io/substrait/expression/VarCharLiteralTest.java @@ -0,0 +1,60 @@ +package io.substrait.expression; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.substrait.TestBase; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +class VarCharLiteralTest extends TestBase { + + @ParameterizedTest + @CsvSource({"abc, 3", "abc, 5", "'a😀', 2", "'é', 2", "'', 0"}) + void fittingValuesRoundTrip(String value, int length) { + for (boolean nullable : new boolean[] {false, true}) { + Expression.VarCharLiteral literal = ExpressionCreator.varChar(nullable, value, length); + assertEquals(value, literal.value()); + assertEquals(length, literal.length()); + assertEquals(nullable, literal.nullable()); + verifyRoundTrip(literal); + } + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void builderRejectsOverlongValue(boolean nullable) { + assertThrows( + IllegalArgumentException.class, + () -> + Expression.VarCharLiteral.builder() + .nullable(nullable) + .value("abcdef") + .length(3) + .build()); + } + + @ParameterizedTest + @CsvSource({"abcdef, 3", "'a😀', 1", "'é', 1"}) + void factoryRejectsOverlongValue(String value, int length) { + assertThrows( + IllegalArgumentException.class, () -> ExpressionCreator.varChar(false, value, length)); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void protoRejectsOverlongValue(boolean nullable) { + io.substrait.proto.Expression proto = + io.substrait.proto.Expression.newBuilder() + .setLiteral( + io.substrait.proto.Expression.Literal.newBuilder() + .setNullable(nullable) + .setVarChar( + io.substrait.proto.Expression.Literal.VarChar.newBuilder() + .setValue("abcdef") + .setLength(3))) + .build(); + assertThrows(IllegalArgumentException.class, () -> protoExpressionConverter.from(proto)); + } +} diff --git a/core/src/test/java/io/substrait/type/proto/GenericRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/GenericRoundtripTest.java index 3e3f42646..fa905ef30 100644 --- a/core/src/test/java/io/substrait/type/proto/GenericRoundtripTest.java +++ b/core/src/test/java/io/substrait/type/proto/GenericRoundtripTest.java @@ -88,6 +88,11 @@ private static List instantiateParams(Method m) } l.add(valGenerator(param)); } + if (m.getReturnType().equals(Expression.VarCharLiteral.class)) { + // The declared width must fit the generated value, rather than be an independent integer. + String value = (String) l.get(1); + l.set(2, value.codePointCount(0, value.length())); + } return l; } diff --git a/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java index 5ccf12452..d7e6318a4 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java @@ -117,23 +117,27 @@ void nullableLiteralBecomesNullabilityCast() { /** * A cast doing more than nullability is not this representation, so the tuple unwrap leaves it * alone -- and a row whose values are not literals to Calcite is not a Values row at all, so it - * takes the projection encoding with the cast still on it. The literal here is malformed input: - * its value is longer than the length it declares, and nothing rejects it, here or in the POJO - * that holds it. What this pins is only the cast, which is the part the conversion decides. + * takes the projection encoding with the cast still on it. */ @Test void tupleUnwrapLeavesATruncatingCastAlone() { - VirtualTableScan overlong = + VirtualTableScan castRow = VirtualTableScan.builder() .initialSchema(NamedStruct.of(List.of("A"), R.struct(R.varChar(3)))) .addRows( ExpressionCreator.nestedStruct( - false, ExpressionCreator.varChar(false, "abcdef", 3))) + false, + ExpressionCreator.cast( + R.varChar(3), + ExpressionCreator.varChar(false, "abcdef", 6), + Expression.FailureBehavior.THROW_EXCEPTION))) .build(); + RelNode calcite = substraitToCalcite.convert(castRow); assertEquals( - "VirtualTable(rows=[[{ CAST('abcdef'):VARCHAR(3) NOT NULL }]])\n", - RelOptUtil.toString(substraitToCalcite.convert(overlong))); + "VirtualTable(rows=[[{ CAST('abcdef':VARCHAR(6)):VARCHAR(3) NOT NULL }]])\n", + RelOptUtil.toString(calcite)); + assertEquals(castRow, SubstraitRelVisitor.convert(calcite, converterProvider)); } /**