fix(isthmus)!: build a character literal from the type its conversion produced

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

@@ -150,7 +150,15 @@ private Type toSubstrait(RelDataType type, List<String> 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) {

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

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"));

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) {