From c17563e349379d5753856b615a1134caad068dff Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 26 Aug 2026 03:35:30 +0300 Subject: [PATCH 01/11] fix(isthmus): keep the declared length of a wide character or binary type SubstraitTypeSystem overrides getMaxPrecision for the temporal types and DECIMAL; CHAR, VARCHAR and BINARY fell through to RelDataTypeSystemImpl, whose default caps them at 65536. The type factory then narrowed anything wider without raising an error, so a plan declaring varchar<2147483647> came back as VARCHAR(65536) and a consumer building a Calcite tree from it had no way to notice. fixedchar and fixed_binary lose their length the same way. Those three are the types whose length crosses the boundary, and Substrait declares each as a 32-bit integer, so they now report Integer.MAX_VALUE. VARBINARY is left alone: Substrait's binary carries no length for it to lose. Closes #1167. --- .../isthmus/SubstraitTypeSystem.java | 12 ++++++ .../io/substrait/isthmus/CalciteTypeTest.java | 37 +++++++++++++++++++ .../isthmus/SubstraitTypeSystemTest.java | 31 ++++++++++++++++ 3 files changed, 80 insertions(+) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 0949584b0..06b31f176 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -81,12 +81,24 @@ public static void requireSupportedPrecision( /** * Returns the maximum precision for the given SQL type. * + *

For the three types that carry a length across the Substrait boundary — {@link + * SqlTypeName#CHAR}, {@link SqlTypeName#VARCHAR} and {@link SqlTypeName#BINARY}, holding {@code + * fixedchar}, {@code varchar} and {@code fixed_binary} — this is Substrait's own limit: those + * lengths are 32-bit integers. Calcite's default of 65536 is narrower, and the type factory caps + * a converted type at it rather than reporting that it cannot represent the declared width. + * {@link SqlTypeName#VARBINARY} is left alone because Substrait's {@code binary} carries no + * length for it to lose. + * * @param typeName The {@link SqlTypeName} for which precision is requested. * @return Maximum precision for the type. */ @Override public int getMaxPrecision(final SqlTypeName typeName) { switch (typeName) { + case CHAR: + case VARCHAR: + case BINARY: + return Integer.MAX_VALUE; case INTERVAL_DAY: case INTERVAL_YEAR: case INTERVAL_YEAR_MONTH: diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java index 50199128f..30861bcd7 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java @@ -15,6 +15,7 @@ import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; class CalciteTypeTest extends CalciteObjs { @@ -220,6 +221,42 @@ void aJavaCharColumnTakesCalcitesDefaultWidth() { TypeConverter.DEFAULT.toSubstrait(javaTypeFactory.createJavaType(Character.class))); } + /** + * A width above Calcite's default 65536 cap, for each type whose length crosses the Substrait + * boundary. The expected precision is asserted directly rather than through {@link #testType}, + * whose expectation is built with the same type factory and would be narrowed alongside the value + * under test. + */ + @ParameterizedTest + @CsvSource({ + "CHAR, 65537", + "CHAR, 2147483647", + "VARCHAR, 65537", + "VARCHAR, 2147483647", + "BINARY, 65537", + "BINARY, 2147483647" + }) + void wideLengthCarryingTypesKeepTheirLength(SqlTypeName typeName, int length) { + TypeExpression substrait; + switch (typeName) { + case CHAR: + substrait = TypeCreator.REQUIRED.fixedChar(length); + break; + case VARCHAR: + substrait = TypeCreator.REQUIRED.varChar(length); + break; + default: + substrait = TypeCreator.REQUIRED.fixedBinary(length); + break; + } + + RelDataType calcite = TypeConverter.DEFAULT.toCalcite(type, substrait, null); + + assertEquals(typeName, calcite.getSqlTypeName()); + assertEquals(length, calcite.getPrecision()); + assertEquals(substrait, TypeConverter.DEFAULT.toSubstrait(calcite)); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void decimal(boolean nullable) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 01e7bd0c7..fc1968448 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -42,6 +42,37 @@ void timeMaxPrecision() { assertEquals(6, typeSystem.getMaxPrecision(SqlTypeName.TIME)); } + @Test + void lengthCarryingTypesMaxPrecisionIsSubstraitsOwnLimit() { + assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.VARCHAR)); + assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.CHAR)); + assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.BINARY)); + } + + /** Substrait's {@code binary} carries no length, so VARBINARY has none to lose. */ + @Test + void varbinaryKeepsTheCalciteDefault() { + assertEquals(65536, typeSystem.getMaxPrecision(SqlTypeName.VARBINARY)); + } + + /** + * Calcite's default caps a character type at 65536, which is narrower than the {@code int} length + * Substrait declares, so a wider converted type would be silently narrowed by the type factory. + */ + @Test + void lengthCarryingTypesMaxPrecisionDiffersFromDefaultTypeSystem() { + assertEquals(65536, RelDataTypeSystem.DEFAULT.getMaxPrecision(SqlTypeName.VARCHAR)); + assertEquals(65536, RelDataTypeSystem.DEFAULT.getMaxPrecision(SqlTypeName.CHAR)); + assertEquals(65536, RelDataTypeSystem.DEFAULT.getMaxPrecision(SqlTypeName.BINARY)); + } + + @Test + void canCreateCharacterTypesWiderThanTheCalciteDefault() { + assertEquals(100_000, TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, 100_000).getPrecision()); + assertEquals(100_000, TYPE_FACTORY.createSqlType(SqlTypeName.CHAR, 100_000).getPrecision()); + assertEquals(100_000, TYPE_FACTORY.createSqlType(SqlTypeName.BINARY, 100_000).getPrecision()); + } + @Test void canCreateDecimalWithMaxPrecision() { RelDataType decimalType = TYPE_FACTORY.createSqlType(SqlTypeName.DECIMAL, 38, 10); From 3d3c9435acaad43e3d3e2a17ae53dcd2e5ac9928 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 26 Aug 2026 16:15:14 +0300 Subject: [PATCH 02/11] test(isthmus): pin the declared width of a SQL cast The cap reaches the conversion from SQL too, which the tests did not say: Calcite narrows a declared width to its maximum silently, so a cast wider than the default used to leave the conversion as a varchar<65536>. --- .../isthmus/SubstraitTypeSystemTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index fc1968448..d11298851 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -3,6 +3,11 @@ import static io.substrait.isthmus.SubstraitTypeSystem.TYPE_FACTORY; import static org.junit.jupiter.api.Assertions.assertEquals; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.plan.Plan; +import io.substrait.type.TypeCreator; +import java.util.List; +import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeSystem; import org.apache.calcite.sql.type.SqlTypeName; @@ -91,4 +96,22 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { assertEquals(38, typeSystem.getMaxPrecision(SqlTypeName.DECIMAL)); assertEquals(38, typeSystem.getMaxScale(SqlTypeName.DECIMAL)); } + + /** + * The cap reaches the conversion from SQL as well. Calcite narrows a declared width to its + * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than + * the default came out of the conversion as a {@code varchar<65536>}. + */ + @Test + void aWideVarcharDeclaredInSqlKeepsItsLength() throws Exception { + CalciteCatalogReader catalog = + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "CREATE TABLE t (a VARCHAR(10))"); + + Plan plan = new SqlToSubstrait().convert("SELECT CAST(a AS VARCHAR(100000)) FROM t", catalog); + + assertEquals( + List.of(TypeCreator.NULLABLE.varChar(100000)), + plan.getRoots().get(0).getInput().getRecordType().fields()); + } } From e96e5126b7fee4b6b051efd0c386abcb1880d1c6 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 28 Aug 2026 11:26:59 +0300 Subject: [PATCH 03/11] fix(isthmus): report a type factory that cannot hold a declared length The conversion takes whatever RelDataTypeFactory it is handed, and a factory caps a width at its type system's maximum without saying so, so raising the maximum fixes the narrowing only for the factory isthmus builds. A fixedchar, varchar or fixed_binary the factory narrows is now reported instead of returned. --- .../io/substrait/isthmus/TypeConverter.java | 30 +++++++++++++++++-- .../isthmus/SubstraitTypeSystemTest.java | 28 +++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index 754387694..f90b3f9ca 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -376,17 +376,41 @@ public RelDataType visit(Type.IntervalDay expr) { @Override public RelDataType visit(Type.FixedChar expr) { - return t(n(expr), SqlTypeName.CHAR, expr.length()); + return withLength(n(expr), SqlTypeName.CHAR, expr.length()); } @Override public RelDataType visit(Type.VarChar expr) { - return t(n(expr), SqlTypeName.VARCHAR, expr.length()); + return withLength(n(expr), SqlTypeName.VARCHAR, expr.length()); } @Override public RelDataType visit(Type.FixedBinary expr) { - return t(n(expr), SqlTypeName.BINARY, expr.length()); + return withLength(n(expr), SqlTypeName.BINARY, expr.length()); + } + + /** + * Returns the type the given factory builds for a declared length, having checked that it holds + * it. A factory caps a width at its type system's maximum without saying so, and this + * conversion takes whatever factory it is handed, so a factory whose limits are not Substrait's + * would otherwise return a type narrower than the plan declares. + * + * @param nullable whether the type is nullable + * @param typeName the Calcite type name to build + * @param length the declared length + * @return the built type + * @throws IllegalArgumentException if the factory built a type of another length + */ + private RelDataType withLength(boolean nullable, SqlTypeName typeName, int length) { + RelDataType type = t(nullable, typeName, length); + if (type.getPrecision() != length) { + throw new IllegalArgumentException( + String.format( + "The type factory cannot hold %s(%d), which it narrowed to %s; its type system" + + " allows up to %d", + typeName, length, type, typeFactory.getTypeSystem().getMaxPrecision(typeName))); + } + return type; } @Override diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index d11298851..bb1065e5d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -2,6 +2,8 @@ import static io.substrait.isthmus.SubstraitTypeSystem.TYPE_FACTORY; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; import io.substrait.plan.Plan; @@ -9,7 +11,9 @@ import java.util.List; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeSystem; +import org.apache.calcite.sql.type.SqlTypeFactoryImpl; import org.apache.calcite.sql.type.SqlTypeName; import org.junit.jupiter.api.Test; @@ -78,6 +82,30 @@ void canCreateCharacterTypesWiderThanTheCalciteDefault() { assertEquals(100_000, TYPE_FACTORY.createSqlType(SqlTypeName.BINARY, 100_000).getPrecision()); } + /** + * The conversion takes whatever type factory it is handed, and one built on Calcite's default + * type system cannot hold these widths. Narrowing them is what this fix is about, so a factory + * that would narrow is reported rather than followed. + */ + @Test + void aFactoryThatCannotHoldTheDeclaredLengthIsReported() { + RelDataTypeFactory defaultFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT); + + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + defaultFactory, TypeCreator.REQUIRED.varChar(100_000), null)); + assertTrue(e.getMessage().contains("65536"), e.getMessage()); + + assertEquals( + 100_000, + TypeConverter.DEFAULT + .toCalcite(TYPE_FACTORY, TypeCreator.REQUIRED.varChar(100_000), null) + .getPrecision()); + } + @Test void canCreateDecimalWithMaxPrecision() { RelDataType decimalType = TYPE_FACTORY.createSqlType(SqlTypeName.DECIMAL, 38, 10); From d0e031ad03671d23af0f778bed8af8a7b92771a6 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 11:19:43 +0300 Subject: [PATCH 04/11] fix(isthmus): keep the width of a ragged binary union Calcite unifies a union of fixed-width binaries of different widths into a VARBINARY of the widest, so the cap this branch removes from BINARY was still reimposed one step later: leastRestrictive(BINARY(100000), BINARY(5)) returned VARBINARY(65536), narrower than one of its own inputs. Substrait's binary carries no length of its own, which is why the type was left alone before, but the width is lost inside Calcite's type unification rather than at the boundary. --- .../isthmus/SubstraitTypeSystem.java | 11 ++++++-- .../isthmus/SubstraitTypeSystemTest.java | 25 ++++++++++++++----- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 06b31f176..68f12ce60 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -86,8 +86,14 @@ public static void requireSupportedPrecision( * fixedchar}, {@code varchar} and {@code fixed_binary} — this is Substrait's own limit: those * lengths are 32-bit integers. Calcite's default of 65536 is narrower, and the type factory caps * a converted type at it rather than reporting that it cannot represent the declared width. - * {@link SqlTypeName#VARBINARY} is left alone because Substrait's {@code binary} carries no - * length for it to lose. + * + *

{@link SqlTypeName#VARBINARY} is raised with them even though Substrait's {@code binary} + * carries no length of its own, because the cap bites inside Calcite's own type unification: + * {@link #shouldConvertRaggedUnionTypesToVarying()} is true here, so a union of fixed-width + * binaries of different widths is unified as a {@code VARBINARY} of the widest. Left at 65536, + * the least restrictive type of {@code BINARY(100000)} and {@code BINARY(5)} is {@code + * VARBINARY(65536)} -- narrower than one of its own inputs, and the cap this method removes + * reimposed. * * @param typeName The {@link SqlTypeName} for which precision is requested. * @return Maximum precision for the type. @@ -98,6 +104,7 @@ public int getMaxPrecision(final SqlTypeName typeName) { case CHAR: case VARCHAR: case BINARY: + case VARBINARY: return Integer.MAX_VALUE; case INTERVAL_DAY: case INTERVAL_YEAR: diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index bb1065e5d..4f1632853 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -56,12 +56,9 @@ void lengthCarryingTypesMaxPrecisionIsSubstraitsOwnLimit() { assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.VARCHAR)); assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.CHAR)); assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.BINARY)); - } - - /** Substrait's {@code binary} carries no length, so VARBINARY has none to lose. */ - @Test - void varbinaryKeepsTheCalciteDefault() { - assertEquals(65536, typeSystem.getMaxPrecision(SqlTypeName.VARBINARY)); + // Substrait's binary carries no length of its own, but Calcite unifies a ragged binary union + // through this type, so a cap here caps the union. + assertEquals(Integer.MAX_VALUE, typeSystem.getMaxPrecision(SqlTypeName.VARBINARY)); } /** @@ -106,6 +103,22 @@ void aFactoryThatCannotHoldTheDeclaredLengthIsReported() { .getPrecision()); } + /** + * A union of fixed-width binaries of different widths is unified as a VARBINARY, so leaving that + * type at Calcite's default would reimpose the cap the wide types are raised past -- on a type + * wider than one of the union's own inputs. + */ + @Test + void aRaggedBinaryUnionKeepsTheWidestWidth() { + RelDataType wide = TYPE_FACTORY.createSqlType(SqlTypeName.BINARY, 100_000); + RelDataType narrow = TYPE_FACTORY.createSqlType(SqlTypeName.BINARY, 5); + + RelDataType unified = TYPE_FACTORY.leastRestrictive(List.of(wide, narrow)); + + assertEquals(SqlTypeName.VARBINARY, unified.getSqlTypeName()); + assertEquals(100_000, unified.getPrecision()); + } + @Test void canCreateDecimalWithMaxPrecision() { RelDataType decimalType = TYPE_FACTORY.createSqlType(SqlTypeName.DECIMAL, 38, 10); From 73b8fc75034eab70f91ec87204fad3650d48b6d7 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 11:19:54 +0300 Subject: [PATCH 05/11] fix(isthmus): refuse a negative declared length before the factory is asked The check compared the factory's answer with the length asked for, which can only report what the factory survives. With assertions off -- the normal production JVM -- the factory stores a negative width and reports it back, so varChar(-5) certified itself as VARCHAR(-5); with them on, Calcite raised a bare AssertionError in place of the message. A length of -1 is Calcite's unspecified precision besides, so varChar(-1) passed and became a Substrait string, while fixedChar(-1) was reported against a bound its length is below. The floor stops at the negatives. The spec puts a fixedchar's width at 1 or more (spec v0.101.0), but Calcite types the empty character literal as a CHAR(0), so a zero length reaches this conversion from ordinary SQL. --- .../io/substrait/isthmus/TypeConverter.java | 16 +++++++- .../isthmus/SubstraitTypeSystemTest.java | 41 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index f90b3f9ca..a4997f992 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -395,13 +395,27 @@ public RelDataType visit(Type.FixedBinary expr) { * conversion takes whatever factory it is handed, so a factory whose limits are not Substrait's * would otherwise return a type narrower than the plan declares. * + *

A negative length is refused before the factory is asked, because asking tells us nothing: + * with assertions off the factory stores the negative and reports it back, so the width below + * certifies itself, and with them on Calcite raises a bare {@code AssertionError} in place of + * this message. A length of -1 is Calcite's unspecified precision besides, so the factory + * answers with an unparameterised type whose precision equals what was asked for. A zero length + * is left to the factory: the spec puts a fixedchar's width at 1 or more, but Calcite types the + * empty character literal as a {@code CHAR(0)}, so plans carrying one exist. + * * @param nullable whether the type is nullable * @param typeName the Calcite type name to build * @param length the declared length * @return the built type - * @throws IllegalArgumentException if the factory built a type of another length + * @throws IllegalArgumentException if the length is negative, or the factory built a type of + * another length */ private RelDataType withLength(boolean nullable, SqlTypeName typeName, int length) { + if (length < 0) { + throw new IllegalArgumentException( + String.format( + "A %s cannot declare a negative length, and this one is %d", typeName, length)); + } RelDataType type = t(nullable, typeName, length); if (type.getPrecision() != length) { throw new IllegalArgumentException( diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 4f1632853..8be8f681c 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -1,6 +1,7 @@ package io.substrait.isthmus; import static io.substrait.isthmus.SubstraitTypeSystem.TYPE_FACTORY; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -103,6 +104,46 @@ void aFactoryThatCannotHoldTheDeclaredLengthIsReported() { .getPrecision()); } + /** + * Asking the factory first cannot tell a negative width from a width it holds: with assertions + * off it stores the negative and reports it back, and -1 is its unspecified precision besides, so + * the answer equals what was asked for either way. + */ + @Test + void aNegativeLengthIsRefusedBeforeTheFactoryIsAsked() { + assertAll( + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + TYPE_FACTORY, TypeCreator.REQUIRED.varChar(-5), null)); + assertTrue( + e.getMessage().contains("negative length, and this one is -5"), e.getMessage()); + }, + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + TYPE_FACTORY, TypeCreator.REQUIRED.fixedChar(-1), null)); + assertTrue( + e.getMessage().contains("negative length, and this one is -1"), e.getMessage()); + }, + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + TYPE_FACTORY, TypeCreator.REQUIRED.fixedBinary(-5), null)); + assertTrue( + e.getMessage().contains("negative length, and this one is -5"), e.getMessage()); + }); + } + /** * A union of fixed-width binaries of different widths is unified as a VARBINARY, so leaving that * type at Calcite's default would reimpose the cap the wide types are raised past -- on a type From c1b86890abcb44623d57ef779b0542a589f3bda9 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 13:18:30 +0300 Subject: [PATCH 06/11] docs(isthmus): spell the type fixedbinary, not fixed_binary fixed_binary is the dialect enum's FIXED_BINARY. The spec and this repo's own StringTypeVisitor both spell the type itself without the underscore. --- .../src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 68f12ce60..849be1526 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -83,7 +83,7 @@ public static void requireSupportedPrecision( * *

For the three types that carry a length across the Substrait boundary — {@link * SqlTypeName#CHAR}, {@link SqlTypeName#VARCHAR} and {@link SqlTypeName#BINARY}, holding {@code - * fixedchar}, {@code varchar} and {@code fixed_binary} — this is Substrait's own limit: those + * fixedchar}, {@code varchar} and {@code fixedbinary} — this is Substrait's own limit: those * lengths are 32-bit integers. Calcite's default of 65536 is narrower, and the type factory caps * a converted type at it rather than reporting that it cannot represent the declared width. * From 6fbcc12913633b215a32fd54bd5cdf0186c144cc Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 13:18:30 +0300 Subject: [PATCH 07/11] fix(isthmus): report a factory that cannot hold a declared precision The guard added for a declared length left a decimal to the same silent narrowing it was added for: decimal<38,10> through a factory on Calcite's default type system came back DECIMAL(19,10) and read back as a precision the plan never declared. The temporal visits already report this through requireSupportedPrecision, so the decimal one now does too. Both public toCalcite overloads document the IllegalArgumentException the length and precision checks raise, and the length test asserts the bound the message reports rather than a number that also appears in the type it narrowed to. --- .../io/substrait/isthmus/TypeConverter.java | 6 +++++ .../isthmus/SubstraitTypeSystemTest.java | 26 ++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index a4997f992..0f9c1576b 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -226,6 +226,8 @@ private Type toSubstrait(RelDataType type, List names) { * @return Calcite relational type. * @throws UnsupportedOperationException if the expression contains unsupported precision or * user-defined types cannot be mapped. + * @throws IllegalArgumentException if a declared length or precision is negative, or the given + * factory cannot hold it. */ public RelDataType toCalcite( RelDataTypeFactory relDataTypeFactory, TypeExpression typeExpression) { @@ -242,6 +244,8 @@ public RelDataType toCalcite( * @return Calcite relational type. * @throws UnsupportedOperationException if the expression contains unsupported precision or * user-defined types cannot be mapped. + * @throws IllegalArgumentException if a declared length or precision is negative, or the given + * factory cannot hold it. */ public RelDataType toCalcite( RelDataTypeFactory relDataTypeFactory, @@ -429,6 +433,8 @@ private RelDataType withLength(boolean nullable, SqlTypeName typeName, int lengt @Override public RelDataType visit(Type.Decimal expr) { + SubstraitTypeSystem.requireSupportedPrecision( + typeFactory.getTypeSystem(), SqlTypeName.DECIMAL, "decimal", expr.precision()); return t(n(expr), SqlTypeName.DECIMAL, expr.precision(), expr.scale()); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 8be8f681c..8bf119cd5 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -95,7 +95,7 @@ void aFactoryThatCannotHoldTheDeclaredLengthIsReported() { () -> TypeConverter.DEFAULT.toCalcite( defaultFactory, TypeCreator.REQUIRED.varChar(100_000), null)); - assertTrue(e.getMessage().contains("65536"), e.getMessage()); + assertTrue(e.getMessage().contains("allows up to 65536"), e.getMessage()); assertEquals( 100_000, @@ -184,6 +184,30 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than * the default came out of the conversion as a {@code varchar<65536>}. */ + /** + * A decimal loses its precision to a foreign factory the same silent way a length does: Calcite's + * default type system caps it at 19, and the narrowed type reads back as one the plan never + * declared. + */ + @Test + void aFactoryThatCannotHoldTheDeclaredPrecisionIsReported() { + RelDataTypeFactory defaultFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT); + + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + defaultFactory, TypeCreator.REQUIRED.decimal(38, 10), null)); + assertTrue(e.getMessage().contains("is set to 19"), e.getMessage()); + + assertEquals( + 38, + TypeConverter.DEFAULT + .toCalcite(TYPE_FACTORY, TypeCreator.REQUIRED.decimal(38, 10), null) + .getPrecision()); + } + @Test void aWideVarcharDeclaredInSqlKeepsItsLength() throws Exception { CalciteCatalogReader catalog = From 7db1e65dbf6e34e64873a9446b16f7f543647116 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 13:18:40 +0300 Subject: [PATCH 08/11] test(isthmus): pin what the raised maximum changes for a concatenation getMaxPrecision(VARCHAR) is also Calcite's overflow threshold in ReturnTypes.DYADIC_STRING_SUM_PRECISION, so raising it changes the emitted type for plans carrying no wide type at all: a || b over two varchar<40000> columns converts to varchar<80000> where it converted to string. The type-mapping switch in the parameterized conversion test throws on an unmatched name rather than falling through to fixedBinary, so a row added for another type fails as the test's own gap rather than as a defect in the type system. --- .../io/substrait/isthmus/CalciteTypeTest.java | 4 +++- .../isthmus/SubstraitTypeSystemTest.java | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java index 30861bcd7..167375636 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java @@ -245,9 +245,11 @@ void wideLengthCarryingTypesKeepTheirLength(SqlTypeName typeName, int length) { case VARCHAR: substrait = TypeCreator.REQUIRED.varChar(length); break; - default: + case BINARY: substrait = TypeCreator.REQUIRED.fixedBinary(length); break; + default: + throw new IllegalArgumentException("no Substrait type mapped for " + typeName); } RelDataType calcite = TypeConverter.DEFAULT.toCalcite(type, substrait, null); diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 8bf119cd5..eb04349ff 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -184,6 +184,25 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than * the default came out of the conversion as a {@code varchar<65536>}. */ + /** + * The raised maximum is also Calcite's overflow threshold for concatenation, in {@code + * ReturnTypes.DYADIC_STRING_SUM_PRECISION}: a sum of widths past it falls back to an + * unparameterised type. So two columns nowhere near the old cap decide the result type between + * them, and the conversion emitted a {@code string} for them before. + */ + @Test + void concatenatingTwoVarcharsKeepsTheSumOfTheirWidths() throws Exception { + CalciteCatalogReader catalog = + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "CREATE TABLE t (a VARCHAR(40000), b VARCHAR(40000))"); + + Plan plan = new SqlToSubstrait().convert("SELECT a || b FROM t", catalog); + + assertEquals( + List.of(TypeCreator.NULLABLE.varChar(80000)), + plan.getRoots().get(0).getInput().getRecordType().fields()); + } + /** * A decimal loses its precision to a foreign factory the same silent way a length does: Calcite's * default type system caps it at 19, and the narrowed type reads back as one the plan never From e691d9744b09659d7abc9ad0c985139edb67b6bd Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 13:23:35 +0300 Subject: [PATCH 09/11] fix(isthmus): refuse a decimal parameter no factory would keep Found checking the ends of the precision guard rather than in review. A negative precision went the way varChar(-1) did: -1 is Calcite's unspecified precision, so decimal<-1,0> converted to a bare DECIMAL and read back as the type system's maximum, through any factory rather than only a foreign one. A scale past the maximum was narrowed the same silent way a precision was -- decimal<19,25> through a factory on Calcite's default type system came back with a scale of 19. Calcite reports a zero precision and a negative scale itself, so the guards here are the two ends it leaves. --- .../isthmus/SubstraitTypeSystem.java | 19 ++++++++++ .../io/substrait/isthmus/TypeConverter.java | 10 +++++ .../isthmus/SubstraitTypeSystemTest.java | 38 +++++++++++++++++++ 3 files changed, 67 insertions(+) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 849be1526..6eef55544 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -78,6 +78,25 @@ public static void requireSupportedPrecision( } } + /** + * Checks that a Substrait decimal scale is one the Calcite type system in effect allows, and + * reports the bound it exceeds if it is not. + * + * @param typeSystem the type system the converted type will live under, which need not be this + * one + * @param scale the scale carried by the Substrait type + * @throws IllegalArgumentException if the scale exceeds what the type system allows + */ + public static void requireSupportedScale(final RelDataTypeSystem typeSystem, final int scale) { + int maxScale = typeSystem.getMaxScale(SqlTypeName.DECIMAL); + if (scale > maxScale) { + throw new IllegalArgumentException( + String.format( + "unsupported decimal scale %s, max scale in Calcite type system is set to %s", + scale, maxScale)); + } + } + /** * Returns the maximum precision for the given SQL type. * diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index 0f9c1576b..cd5b3b424 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -433,8 +433,18 @@ private RelDataType withLength(boolean nullable, SqlTypeName typeName, int lengt @Override public RelDataType visit(Type.Decimal expr) { + // Before the factory, for the reason the lengths are: -1 is Calcite's unspecified precision, + // so a negative one is answered with an unparameterised DECIMAL whose precision reads back as + // the type system's maximum. A zero or negative scale Calcite reports itself. + if (expr.precision() < 0) { + throw new IllegalArgumentException( + String.format( + "A decimal cannot declare a negative precision, and this one is %d", + expr.precision())); + } SubstraitTypeSystem.requireSupportedPrecision( typeFactory.getTypeSystem(), SqlTypeName.DECIMAL, "decimal", expr.precision()); + SubstraitTypeSystem.requireSupportedScale(typeFactory.getTypeSystem(), expr.scale()); return t(n(expr), SqlTypeName.DECIMAL, expr.precision(), expr.scale()); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index eb04349ff..77ab88e60 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -184,6 +184,44 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than * the default came out of the conversion as a {@code varchar<65536>}. */ + /** + * The same two ends as a length, for the same reasons: -1 is Calcite's unspecified precision, so + * the factory answers with an unparameterised DECIMAL whose precision reads back as the type + * system's maximum, and a scale past the maximum is narrowed rather than reported. Calcite + * reports a zero or negative scale and a zero precision itself. + */ + @Test + void aDecimalParameterOutsideWhatTheFactoryHoldsIsRefused() { + RelDataTypeFactory defaultFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT); + + assertAll( + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + TYPE_FACTORY, TypeCreator.REQUIRED.decimal(-1, 0), null)); + assertTrue( + e.getMessage().contains("negative precision, and this one is -1"), e.getMessage()); + }, + () -> { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + defaultFactory, TypeCreator.REQUIRED.decimal(19, 25), null)); + assertTrue(e.getMessage().contains("max scale"), e.getMessage()); + }, + () -> + assertEquals( + 25, + TypeConverter.DEFAULT + .toCalcite(TYPE_FACTORY, TypeCreator.REQUIRED.decimal(19, 25), null) + .getScale())); + } + /** * The raised maximum is also Calcite's overflow threshold for concatenation, in {@code * ReturnTypes.DYADIC_STRING_SUM_PRECISION}: a sum of widths past it falls back to an From a9e7ae97e7a8e4f1eb90c05fc14a0456028c97ad Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 15:49:27 +0300 Subject: [PATCH 10/11] fix(isthmus): bound a decimal's scale by its precision, not by the factory The scale check compared against the type system's maxScale, which no spec-valid decimal can exceed: the spec puts scale at 0 <= S <= P <= 38, and both type systems here set maxScale equal to maxPrecision, so a scale within the precision is within the maximum too. The bound that was missing is the spec's own -- decimal<19,25> has a scale above its precision, and Calcite built DECIMAL(19,25) from it without complaint. The javadoc of the SQL-cast test goes back to that test as well; it was left behind when the decimal test was added above it. --- .../isthmus/SubstraitTypeSystem.java | 19 ----------- .../io/substrait/isthmus/TypeConverter.java | 10 +++++- .../isthmus/SubstraitTypeSystemTest.java | 32 +++++++++---------- 3 files changed, 25 insertions(+), 36 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 6eef55544..849be1526 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -78,25 +78,6 @@ public static void requireSupportedPrecision( } } - /** - * Checks that a Substrait decimal scale is one the Calcite type system in effect allows, and - * reports the bound it exceeds if it is not. - * - * @param typeSystem the type system the converted type will live under, which need not be this - * one - * @param scale the scale carried by the Substrait type - * @throws IllegalArgumentException if the scale exceeds what the type system allows - */ - public static void requireSupportedScale(final RelDataTypeSystem typeSystem, final int scale) { - int maxScale = typeSystem.getMaxScale(SqlTypeName.DECIMAL); - if (scale > maxScale) { - throw new IllegalArgumentException( - String.format( - "unsupported decimal scale %s, max scale in Calcite type system is set to %s", - scale, maxScale)); - } - } - /** * Returns the maximum precision for the given SQL type. * diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index cd5b3b424..4e6ecd1d2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -444,7 +444,15 @@ public RelDataType visit(Type.Decimal expr) { } SubstraitTypeSystem.requireSupportedPrecision( typeFactory.getTypeSystem(), SqlTypeName.DECIMAL, "decimal", expr.precision()); - SubstraitTypeSystem.requireSupportedScale(typeFactory.getTypeSystem(), expr.scale()); + // The spec puts a decimal's scale in [0..P]. No factory reports a scale above the precision: + // Calcite builds the type as asked, and its own maximum cannot catch it either, since both + // type systems here set maxScale equal to maxPrecision. + if (expr.scale() > expr.precision()) { + throw new IllegalArgumentException( + String.format( + "A decimal cannot declare a scale of %d above its precision of %d", + expr.scale(), expr.precision())); + } return t(n(expr), SqlTypeName.DECIMAL, expr.precision(), expr.scale()); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 77ab88e60..2f4baa4d0 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -180,20 +180,15 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { } /** - * The cap reaches the conversion from SQL as well. Calcite narrows a declared width to its - * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than - * the default came out of the conversion as a {@code varchar<65536>}. - */ - /** - * The same two ends as a length, for the same reasons: -1 is Calcite's unspecified precision, so - * the factory answers with an unparameterised DECIMAL whose precision reads back as the type - * system's maximum, and a scale past the maximum is narrowed rather than reported. Calcite - * reports a zero or negative scale and a zero precision itself. + * Two ends the factory does not report. A precision of -1 is Calcite's unspecified precision, so + * it answers with an unparameterised DECIMAL whose precision reads back as the type system's + * maximum. A scale above the precision is outside the spec's {@code 0 <= S <= P} and Calcite + * builds it anyway, where its own maximum would not catch it: both type systems here set {@code + * maxScale} equal to {@code maxPrecision}, so a scale within the precision is within that too. + * Calcite reports a zero or negative scale and a zero precision itself. */ @Test - void aDecimalParameterOutsideWhatTheFactoryHoldsIsRefused() { - RelDataTypeFactory defaultFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT); - + void aDecimalParameterOutsideItsDeclaredBoundsIsRefused() { assertAll( () -> { IllegalArgumentException e = @@ -211,14 +206,14 @@ void aDecimalParameterOutsideWhatTheFactoryHoldsIsRefused() { IllegalArgumentException.class, () -> TypeConverter.DEFAULT.toCalcite( - defaultFactory, TypeCreator.REQUIRED.decimal(19, 25), null)); - assertTrue(e.getMessage().contains("max scale"), e.getMessage()); + TYPE_FACTORY, TypeCreator.REQUIRED.decimal(19, 25), null)); + assertTrue(e.getMessage().contains("scale of 25 above its precision"), e.getMessage()); }, () -> assertEquals( - 25, + 19, TypeConverter.DEFAULT - .toCalcite(TYPE_FACTORY, TypeCreator.REQUIRED.decimal(19, 25), null) + .toCalcite(TYPE_FACTORY, TypeCreator.REQUIRED.decimal(19, 19), null) .getScale())); } @@ -265,6 +260,11 @@ void aFactoryThatCannotHoldTheDeclaredPrecisionIsReported() { .getPrecision()); } + /** + * The cap reaches the conversion from SQL as well. Calcite narrows a declared width to its + * maximum silently rather than reporting that it cannot hold it, so before this a cast wider than + * the default came out of the conversion as a {@code varchar<65536>}. + */ @Test void aWideVarcharDeclaredInSqlKeepsItsLength() throws Exception { CalciteCatalogReader catalog = From 0c54c0c224094e41da1259569ac211967741a955 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Tue, 1 Sep 2026 16:24:56 +0300 Subject: [PATCH 11/11] fix(isthmus): bound a decimal's precision by the spec's 38 as well The precision was checked only against the type factory's maximum, so a type system that allows more than the spec does let a wider decimal through: with a DECIMAL maximum of 76, decimal<45,2> converted to DECIMAL(45,2) and the outbound conversion then refused it with "unsupported decimal precision 45", leaving a type that converts in and cannot come back. Both ends of that clause now read one constant rather than two spellings of 38. --- .../io/substrait/isthmus/TypeConverter.java | 14 +++++++- .../isthmus/SubstraitTypeSystemTest.java | 32 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java index 4e6ecd1d2..fb2779bd2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java @@ -31,6 +31,9 @@ */ public class TypeConverter { + /** The widest precision the spec gives a decimal: {@code DECIMAL} puts P at 38 or less. */ + private static final int MAX_DECIMAL_PRECISION = 38; + private final UserTypeMapper userTypeMapper; /** @@ -143,7 +146,7 @@ private Type toSubstrait(RelDataType type, List names) { return creator.FP64; case DECIMAL: { - if (type.getPrecision() > 38) { + if (type.getPrecision() > MAX_DECIMAL_PRECISION) { throw new UnsupportedOperationException( "unsupported decimal precision " + type.getPrecision()); } @@ -442,6 +445,15 @@ public RelDataType visit(Type.Decimal expr) { "A decimal cannot declare a negative precision, and this one is %d", expr.precision())); } + // The spec's own ceiling, not just the factory's: handed a type system whose DECIMAL maximum + // is above it, the factory builds the type and the outbound conversion above then refuses it, + // so the type would convert in and have no way back. + if (expr.precision() > MAX_DECIMAL_PRECISION) { + throw new IllegalArgumentException( + String.format( + "A decimal cannot declare a precision of %d, above the %d the spec allows", + expr.precision(), MAX_DECIMAL_PRECISION)); + } SubstraitTypeSystem.requireSupportedPrecision( typeFactory.getTypeSystem(), SqlTypeName.DECIMAL, "decimal", expr.precision()); // The spec puts a decimal's scale in [0..P]. No factory reports a scale above the precision: diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 2f4baa4d0..03dd4c77f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -14,6 +14,7 @@ import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeSystem; +import org.apache.calcite.rel.type.RelDataTypeSystemImpl; import org.apache.calcite.sql.type.SqlTypeFactoryImpl; import org.apache.calcite.sql.type.SqlTypeName; import org.junit.jupiter.api.Test; @@ -179,6 +180,37 @@ void decimalMaxPrecisionAndScaleDifferentFromDefaultTypeSystem() { assertEquals(38, typeSystem.getMaxScale(SqlTypeName.DECIMAL)); } + /** + * A type system whose DECIMAL maximum is above the spec's 38 would let a wider decimal through + * the factory, and {@code toSubstrait} refuses it on the way back -- so the type would convert in + * with no way out. The bound is the spec's rather than the factory's for that reason. + */ + @Test + void aDecimalPrecisionAboveTheSpecsCeilingIsRefused() { + RelDataTypeFactory wideFactory = + new SqlTypeFactoryImpl( + new RelDataTypeSystemImpl() { + @Override + public int getMaxPrecision(SqlTypeName typeName) { + return typeName == SqlTypeName.DECIMAL ? 76 : super.getMaxPrecision(typeName); + } + + @Override + public int getMaxScale(SqlTypeName typeName) { + return typeName == SqlTypeName.DECIMAL ? 76 : super.getMaxScale(typeName); + } + }); + + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + TypeConverter.DEFAULT.toCalcite( + wideFactory, TypeCreator.REQUIRED.decimal(45, 2), null)); + + assertTrue(e.getMessage().contains("above the 38 the spec allows"), e.getMessage()); + } + /** * Two ends the factory does not report. A precision of -1 is Calcite's unspecified precision, so * it answers with an unparameterised DECIMAL whose precision reads back as the type system's