fix(isthmus)!: keep the declared length of a wide character or binary type

isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java

@@ -81,12 +81,31 @@ public static void requireSupportedPrecision(
   /**
    * Returns the maximum precision for the given SQL type.
    *
+   * <p>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 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.
+   *
+   * <p>{@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.
    */
   @Override
   public int getMaxPrecision(final SqlTypeName typeName) {
     switch (typeName) {
+      case CHAR:
+      case VARCHAR:
+      case BINARY:
+      case VARBINARY:
+        return Integer.MAX_VALUE;
       case INTERVAL_DAY:
       case INTERVAL_YEAR:
       case INTERVAL_YEAR_MONTH:

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<P, S>} 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<String> names) {
         return creator.FP64;
       case DECIMAL:
         {
-          if (type.getPrecision() > 38) {
+          if (type.getPrecision() > MAX_DECIMAL_PRECISION) {
             throw new UnsupportedOperationException(
                 "unsupported decimal precision " + type.getPrecision());
           }
@@ -226,6 +229,8 @@ private Type toSubstrait(RelDataType type, List<String> 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 +247,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,
@@ -376,21 +383,88 @@ 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.
+     *
+     * <p>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 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(
+            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
     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()));
+      }
+      // 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:
+      // 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());
     }
 

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,44 @@ 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;
+      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);
+
+    assertEquals(typeName, calcite.getSqlTypeName());
+    assertEquals(length, calcite.getPrecision());
+    assertEquals(substrait, TypeConverter.DEFAULT.toSubstrait(calcite));
+  }
+
   @ParameterizedTest
   @ValueSource(booleans = {true, false})
   void decimal(boolean nullable) {