fix(isthmus): use Substrait type system when building Calcite RelBuilder (#973)

When converting Substrait to Calcite, the `RelBuilder` was created via
`RelBuilder.create(Frameworks...)` without specifying a type system, so
its internal `RexBuilder` used Calcite's default type system (max
decimal precision 19) while the converted expressions carried
`SubstraitTypeSystem` types (precision 38). `RelBuilder.project` runs
`RexSimplify` by default; with the mismatched type system it re-derived
decimal arithmetic at precision 19 and then wrapped the result in a
preserving `CAST(... AS DECIMAL(38,0))` to keep the declared type.

For TPC-H queries 8 and 14 this surfaced as the numerator `SUM(CAST(CASE
... AS DECIMAL(19,0)))` while the denominator was left uncast. In
PostgreSQL, where the `DECIMAL` columns are arbitrary-precision
`NUMERIC`, the cast truncated the fractional values in the numerator
only, so the round-tripped SQL produced different results from the
reference. This was latent until Calcite 1.42, which changed
`RelDataTypeSystemImpl` so the max numeric precision/scale are derived
from `getMaxPrecision(DECIMAL)`/`getMaxScale(DECIMAL)`.

## Changes

- Build the Substrait-to-Calcite `RelBuilder` with the Substrait type
system, exposed via a new `ConverterProvider.getTypeSystem()` accessor
(derived from the type factory so the two never disagree).
- `SubstraitTypeSystem` gains a public no-arg constructor because
Calcite's `Frameworks`/Avatica machinery re-instantiates the type system
from its class name when it is supplied to a `FrameworkConfig`.
- Removed queries 8 and 14 from the integration test's expected-failures
list, and dropped the now-empty expected-failures code path.

## Testing

- `./gradlew :isthmus:test` — all unit tests pass.
- `./gradlew :isthmus:integrationTest` — all 22 TPC-H queries (including
8 and 14) now produce results matching the reference SQL in PostgreSQL.

Closes #954
Closes #955

🤖 Generated with AI

Author: nielspardon

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

@@ -24,6 +24,7 @@
 import org.apache.calcite.jdbc.CalciteSchema;
 import org.apache.calcite.prepare.Prepare;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
 import org.apache.calcite.rex.RexBuilder;
 import org.apache.calcite.sql.SqlOperatorTable;
 import org.apache.calcite.sql.parser.SqlParser;
@@ -332,7 +333,11 @@ public ExpressionRexConverter getExpressionRexConverter(
    * @return a new RelBuilder instance
    */
   public RelBuilder getRelBuilder(CalciteSchema schema) {
-    return RelBuilder.create(Frameworks.newConfigBuilder().defaultSchema(schema.plus()).build());
+    return RelBuilder.create(
+        Frameworks.newConfigBuilder()
+            .defaultSchema(schema.plus())
+            .typeSystem(getTypeSystem())
+            .build());
   }
 
   // Utility Getters
@@ -346,6 +351,19 @@ public RelDataTypeFactory getTypeFactory() {
     return typeFactory;
   }
 
+  /**
+   * Returns the Calcite {@link RelDataTypeSystem} used by this converter provider.
+   *
+   * <p>Derived from the {@link #getTypeFactory() type factory} so the two never disagree. This is
+   * the type system supplied to the {@link RelBuilder} used when converting Substrait to Calcite,
+   * ensuring its type derivation matches the types carried by converted expressions.
+   *
+   * @return the type system
+   */
+  public RelDataTypeSystem getTypeSystem() {
+    return typeFactory.getTypeSystem();
+  }
+
   /**
    * Returns the Substrait extension collection used by this converter provider.
    *

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

@@ -161,6 +161,7 @@ public static RelNode convert(
                 .parserConfig(converterProvider.getSqlParserConfig())
                 .defaultSchema(catalogReader.getRootSchema().plus())
                 .traitDefs((List<RelTraitDef>) null)
+                .typeSystem(converterProvider.getTypeSystem())
                 .programs()
                 .build());
     return relRoot.accept(

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

@@ -32,8 +32,16 @@ public class SubstraitTypeSystem extends RelDataTypeSystemImpl {
   public static final SqlIntervalQualifier DAY_SECOND_INTERVAL =
       new SqlIntervalQualifier(TimeUnit.DAY, -1, TimeUnit.SECOND, 6, SqlParserPos.ZERO);
 
-  /** Private constructor to enforce singleton usage. */
-  private SubstraitTypeSystem() {}
+  /**
+   * Public no-argument constructor.
+   *
+   * <p>Prefer the shared {@link #TYPE_SYSTEM} singleton. This constructor exists because Calcite's
+   * {@link org.apache.calcite.tools.Frameworks}/Avatica machinery re-instantiates a type system
+   * from its class name (via a default constructor) when it is supplied to a {@link
+   * org.apache.calcite.tools.FrameworkConfig}. The type system is stateless, so additional
+   * instances are equivalent to the singleton.
+   */
+  public SubstraitTypeSystem() {}
 
   /**
    * Returns the maximum precision for the given SQL type.

isthmus/src/test/java/io/substrait/isthmus/integration/PostgreSqlIntegrationTest.java

@@ -2,7 +2,6 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import io.substrait.isthmus.ConverterProvider;
@@ -39,9 +38,6 @@
 class PostgreSqlIntegrationTest extends PlanTestBase {
   private static final Logger LOG = LoggerFactory.getLogger(PostgreSqlIntegrationTest.class);
 
-  // TODO: These queries produce different results when generated from Substrait
-  private static final List<Integer> EXPECTED_FAILURES = List.of(8, 14);
-
   private static final DockerImageName UV_IMAGE =
       DockerImageName.parse("ghcr.io/astral-sh/uv:python3.14-trixie-slim");
   private static final DockerImageName POSTGRES_IMAGE = DockerImageName.parse("postgres:18");
@@ -126,20 +122,12 @@ void testTpcH(final int queryNo)
 
       // the count should be zero if both the reference and generated SQL produce the same results
       int differenceCount = result.getInt(1);
-      if (EXPECTED_FAILURES.contains(queryNo)) {
-        assertNotEquals(
-            0,
-            differenceCount,
-            String.format(
-                "Expected query %d to fail but it matched the reference results", queryNo));
-      } else {
-        assertEquals(
-            0,
-            differenceCount,
-            String.format(
-                "Reference and generated SQL produce %d different results.\n\nReference SQL:\n%s\n\nGenerated SQL:\n%s",
-                differenceCount, referenceSql, generatedSql));
-      }
+      assertEquals(
+          0,
+          differenceCount,
+          String.format(
+              "Reference and generated SQL produce %d different results.\n\nReference SQL:\n%s\n\nGenerated SQL:\n%s",
+              differenceCount, referenceSql, generatedSql));
 
       // we expect exactly one row
       assertFalse(result.next());