From dfab9fee3de1ea6e859f99025ea34c6ebe7fdcf9 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 31 Jul 2026 11:06:52 +0200 Subject: [PATCH 1/2] refactor(isthmus): derive CREATE-statement table types from the provider The builder work threaded the ConverterProvider into the CalciteCatalogReader construction, but the type derivation underneath still used global defaults on that same injected path: - createSubstraitTable built the row type with SubstraitTypeSystem.TYPE_FACTORY instead of the provider's type factory. - Column types came from col.dataType.deriveType(VALIDATOR), where VALIDATOR and EMPTY_CATALOG are static finals built on the global type factory and SqlConverterBase.CONNECTION_CONFIG. So a builder-configured typeFactory produced a catalog reader that used it while the tables inside were typed by the global one. Harmless today, since getCalciteConnectionConfig() returns the same case-insensitive config, but a custom type factory carrying a different type system was silently bypassed. Add createEmptyCatalog(ConverterProvider) and createValidator(ConverterProvider) helpers, build the validator once per call, and pass the provider's type factory and that validator into createSubstraitTable. EMPTY_CATALOG and VALIDATOR are retained and now defined via the same helpers against ConverterProvider.DEFAULT, so their values are unchanged. The validator is also built with the provider's SqlOperatorTable rather than a hardcoded SubstraitOperatorTable.INSTANCE, matching how the query path already sources its operator table. Identical for the default provider. CreateStatementParserProviderConfigTest covers both entry points; 3 of its 4 tests fail without this change. --- .../sql/SubstraitCreateStatementParser.java | 83 +++++++++--- ...eateStatementParserProviderConfigTest.java | 121 ++++++++++++++++++ 2 files changed, 184 insertions(+), 20 deletions(-) create mode 100644 isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java index a6b52cd21..bf0c2c6cb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java @@ -1,8 +1,6 @@ package io.substrait.isthmus.sql; import io.substrait.isthmus.ConverterProvider; -import io.substrait.isthmus.SqlConverterBase; -import io.substrait.isthmus.SubstraitTypeSystem; import io.substrait.isthmus.Utils; import io.substrait.isthmus.calcite.SubstraitTable; import java.util.ArrayList; @@ -11,6 +9,7 @@ import org.apache.calcite.jdbc.CalciteSchema; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.sql.SqlNode; import org.apache.calcite.sql.SqlNodeList; import org.apache.calcite.sql.ddl.SqlColumnDeclaration; @@ -25,19 +24,18 @@ /** Utility class for parsing CREATE statements into a {@link CalciteCatalogReader} */ public class SubstraitCreateStatementParser { - /** An empty catalog reader used for validating CREATE statements. */ + /** + * An empty catalog reader used for validating CREATE statements, configured from {@link + * ConverterProvider#DEFAULT}. + */ public static final CalciteCatalogReader EMPTY_CATALOG = - new CalciteCatalogReader( - CalciteSchema.createRootSchema(false), - List.of(), - SubstraitTypeSystem.TYPE_FACTORY, - SqlConverterBase.CONNECTION_CONFIG); - - /** SQL validator configured for validating CREATE statements against the empty catalog. */ - public static final SqlValidator VALIDATOR = - new SubstraitSqlValidator( - // as we are validating CREATE statements, an empty catalog suffices - EMPTY_CATALOG); + createEmptyCatalog(ConverterProvider.DEFAULT); + + /** + * SQL validator configured for validating CREATE statements against the empty catalog, using + * {@link ConverterProvider#DEFAULT}. + */ + public static final SqlValidator VALIDATOR = createValidator(ConverterProvider.DEFAULT); /** * Parses a SQL string containing only CREATE statements into a list of {@link SubstraitTable}s. @@ -71,6 +69,7 @@ public static List processCreateStatements( @NonNull final ConverterProvider converterProvider, @NonNull final String createStatements) throws SqlParseException { final List tableList = new ArrayList<>(); + final SqlValidator validator = createValidator(converterProvider); final List sqlNode = SubstraitSqlStatementParser.parseStatements(createStatements, converterProvider); @@ -89,7 +88,12 @@ public static List processCreateStatements( throw fail("CTAS not supported.", create.name.getParserPosition()); } - tableList.add(createSubstraitTable(create.name.names.get(0), create.columnList)); + tableList.add( + createSubstraitTable( + converterProvider.getTypeFactory(), + validator, + create.name.names.get(0), + create.columnList)); } return tableList; @@ -200,6 +204,7 @@ private static CalciteSchema processCreateStatementsToSchema( @NonNull final ConverterProvider converterProvider, @NonNull final String... createStatements) throws SqlParseException { final CalciteSchema rootSchema = CalciteSchema.createRootSchema(false); + final SqlValidator validator = createValidator(converterProvider); for (final String statement : createStatements) { final List sqlNode = @@ -219,7 +224,10 @@ private static CalciteSchema processCreateStatementsToSchema( final String tableName = names.get(names.size() - 1); final CalciteSchema.TableEntry table = schema.getTable(tableName, false); if (table == null) { - schema.add(tableName, createSubstraitTable(tableName, create.columnList)); + schema.add( + tableName, + createSubstraitTable( + converterProvider.getTypeFactory(), validator, tableName, create.columnList)); } else { throw fail("Table must not be defined more than once", parsed.getParserPosition()); } @@ -233,6 +241,8 @@ private static CalciteSchema processCreateStatementsToSchema( * Creates a new {@link SubstraitTable} with the given table name and the table schema from the * given {@link SqlNodeList} containing {@link SqlColumnDeclaration}s. * + * @param typeFactory the type factory used to build the table's row type; must not be null + * @param validator the validator used to derive the column types; must not be null * @param tableName the table name to use; must not be null * @param columnList the {@link SqlNodeList} containing {@link SqlColumnDeclaration}s to build the * table schema from; must not be null @@ -240,7 +250,10 @@ private static CalciteSchema processCreateStatementsToSchema( * @throws SqlParseException if the column list contains unexpected nodes or invalid names */ private static SubstraitTable createSubstraitTable( - @NonNull final String tableName, @NonNull final SqlNodeList columnList) + @NonNull final RelDataTypeFactory typeFactory, + @NonNull final SqlValidator validator, + @NonNull final String tableName, + @NonNull final SqlNodeList columnList) throws SqlParseException { final List names = new ArrayList<>(); final List columnTypes = new ArrayList<>(); @@ -263,10 +276,40 @@ private static SubstraitTable createSubstraitTable( } names.add(col.name.names.get(0)); - columnTypes.add(col.dataType.deriveType(VALIDATOR)); + columnTypes.add(col.dataType.deriveType(validator)); } - return new SubstraitTable( - tableName, SubstraitTypeSystem.TYPE_FACTORY.createStructType(columnTypes, names)); + return new SubstraitTable(tableName, typeFactory.createStructType(columnTypes, names)); + } + + /** + * Creates an empty catalog reader for validating CREATE statements, using the type factory and + * connection configuration from the given {@link ConverterProvider}. + * + * @param converterProvider the converter provider supplying the type factory and connection + * configuration; must not be null + * @return an empty {@link CalciteCatalogReader} + */ + private static CalciteCatalogReader createEmptyCatalog( + @NonNull final ConverterProvider converterProvider) { + return new CalciteCatalogReader( + CalciteSchema.createRootSchema(false), + List.of(), + converterProvider.getTypeFactory(), + converterProvider.getCalciteConnectionConfig()); + } + + /** + * Creates a SQL validator for deriving the column types of CREATE statements, configured from the + * given {@link ConverterProvider}. As only CREATE statements are validated, an empty catalog + * suffices. + * + * @param converterProvider the converter provider supplying the validator configuration; must not + * be null + * @return a {@link SqlValidator} for validating CREATE statements + */ + private static SqlValidator createValidator(@NonNull final ConverterProvider converterProvider) { + return new SubstraitSqlValidator( + createEmptyCatalog(converterProvider), converterProvider.getSqlOperatorTable()); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java b/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java new file mode 100644 index 000000000..0635c3a77 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java @@ -0,0 +1,121 @@ +package io.substrait.isthmus.sql; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.substrait.isthmus.ConverterProvider; +import io.substrait.isthmus.SubstraitTypeSystem; +import io.substrait.isthmus.calcite.SubstraitTable; +import java.util.List; +import org.apache.calcite.prepare.CalciteCatalogReader; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.type.SqlTypeFactoryImpl; +import org.junit.jupiter.api.Test; + +/** + * Verifies that the CREATE-statement parser sources its type factory and connection configuration + * from the injected {@link ConverterProvider} rather than from global defaults, for both the + * catalog-reader and the {@link SubstraitTable} entry points. + */ +class CreateStatementParserProviderConfigTest { + + private static final String CREATE_STATEMENT = + "CREATE TABLE employees (id BIGINT, name VARCHAR, salary DECIMAL(10, 2))"; + + /** + * A type factory that records the struct types it is asked to build, so a test can tell which + * factory produced a table's row type and where its column types came from. + */ + static final class RecordingTypeFactory extends SqlTypeFactoryImpl { + + private boolean createStructTypeCalled; + private List recordedFieldTypes = List.of(); + + RecordingTypeFactory() { + super(SubstraitTypeSystem.TYPE_SYSTEM); + } + + @Override + public RelDataType createStructType( + final List typeList, final List fieldNameList) { + createStructTypeCalled = true; + recordedFieldTypes = List.copyOf(typeList); + return super.createStructType(typeList, fieldNameList); + } + + boolean wasUsedForStructType() { + return createStructTypeCalled; + } + + List recordedFieldTypes() { + return recordedFieldTypes; + } + + /** Exposes the protected interning hook so a test can check a type belongs to this factory. */ + RelDataType canonizeType(final RelDataType type) { + return canonize(type); + } + } + + @Test + void catalogPathUsesProviderTypeFactory() throws SqlParseException { + RecordingTypeFactory typeFactory = new RecordingTypeFactory(); + ConverterProvider provider = ConverterProvider.builder().typeFactory(typeFactory).build(); + + CalciteCatalogReader catalog = + SubstraitCreateStatementParser.processCreateStatementsToCatalog(provider, CREATE_STATEMENT); + + assertSame(typeFactory, catalog.getTypeFactory()); + assertTrue( + typeFactory.wasUsedForStructType(), + "the provider's type factory should build the table row type"); + } + + @Test + void tableListPathUsesProviderTypeFactory() throws SqlParseException { + RecordingTypeFactory typeFactory = new RecordingTypeFactory(); + ConverterProvider provider = ConverterProvider.builder().typeFactory(typeFactory).build(); + + List tables = + SubstraitCreateStatementParser.processCreateStatements(provider, CREATE_STATEMENT); + + assertEquals(1, tables.size()); + assertTrue( + typeFactory.wasUsedForStructType(), + "the provider's type factory should build the table row type"); + } + + /** + * The column types are derived through a validator built from the provider, so they are interned + * in the provider's type factory too — not just the enclosing struct type. + */ + @Test + void columnTypesAreDerivedWithProviderTypeFactory() throws SqlParseException { + RecordingTypeFactory typeFactory = new RecordingTypeFactory(); + ConverterProvider provider = ConverterProvider.builder().typeFactory(typeFactory).build(); + + SubstraitCreateStatementParser.processCreateStatementsToCatalog(provider, CREATE_STATEMENT); + + List fieldTypes = typeFactory.recordedFieldTypes(); + assertEquals(3, fieldTypes.size()); + for (RelDataType fieldType : fieldTypes) { + assertSame( + fieldType, + typeFactory.canonizeType(fieldType), + "column types should be interned in the provider's type factory"); + } + } + + /** The default entry points keep working off {@link ConverterProvider#DEFAULT}. */ + @Test + void defaultPathStillUsesSystemDefaults() throws SqlParseException { + CalciteCatalogReader catalog = + SubstraitCreateStatementParser.processCreateStatementsToCatalog(CREATE_STATEMENT); + + assertSame(SubstraitTypeSystem.TYPE_FACTORY, catalog.getTypeFactory()); + assertFalse(catalog.nameMatcher().isCaseSensitive(), "default config is case-insensitive"); + } +} From c58bea6bde7c3da92e54bdcce621deaefc8eb2cc Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 3 Aug 2026 10:21:46 +0200 Subject: [PATCH 2/2] test(isthmus): assert the validator path uses the provider's type factory The column-derivation check compared a field type against canonize(...) on the provider's type factory, which proves nothing: RelDataTypeFactoryImpl.canonize interns through a private static Interner shared by every factory instance, so the comparison holds no matter which factory created the type. That test only failed without the fix because its size assertion duplicated the row-type check already covered by the other two tests. Record the SqlTypeNames requested from the factory instead and assert the declared column types are among them, which does exercise the validator path. Matching on the specific names matters: the factory is asked for BOOLEAN, CHAR and NULL while the catalog is set up regardless, so a plain "createSqlType was called" flag would pass even with a global validator. --- ...eateStatementParserProviderConfigTest.java | 62 ++++++++++++------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java b/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java index 0635c3a77..a91770e71 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/sql/CreateStatementParserProviderConfigTest.java @@ -8,11 +8,13 @@ import io.substrait.isthmus.ConverterProvider; import io.substrait.isthmus.SubstraitTypeSystem; import io.substrait.isthmus.calcite.SubstraitTable; +import java.util.ArrayList; import java.util.List; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.sql.type.SqlTypeFactoryImpl; +import org.apache.calcite.sql.type.SqlTypeName; import org.junit.jupiter.api.Test; /** @@ -26,13 +28,18 @@ class CreateStatementParserProviderConfigTest { "CREATE TABLE employees (id BIGINT, name VARCHAR, salary DECIMAL(10, 2))"; /** - * A type factory that records the struct types it is asked to build, so a test can tell which - * factory produced a table's row type and where its column types came from. + * A type factory that records the row types it is asked to build and the SQL types it is asked to + * create, so a test can tell which factory produced a table's row type and which one the + * validator derived its column types through. + * + *

Recording the requests is what makes the column-derivation check meaningful: {@code + * RelDataTypeFactoryImpl.canonize} interns through a {@code static} cache shared by every factory + * instance, so comparing interned type instances cannot distinguish one factory from another. */ static final class RecordingTypeFactory extends SqlTypeFactoryImpl { private boolean createStructTypeCalled; - private List recordedFieldTypes = List.of(); + private final List requestedSqlTypeNames = new ArrayList<>(); RecordingTypeFactory() { super(SubstraitTypeSystem.TYPE_SYSTEM); @@ -42,21 +49,34 @@ static final class RecordingTypeFactory extends SqlTypeFactoryImpl { public RelDataType createStructType( final List typeList, final List fieldNameList) { createStructTypeCalled = true; - recordedFieldTypes = List.copyOf(typeList); return super.createStructType(typeList, fieldNameList); } - boolean wasUsedForStructType() { - return createStructTypeCalled; + @Override + public RelDataType createSqlType(final SqlTypeName typeName) { + requestedSqlTypeNames.add(typeName); + return super.createSqlType(typeName); } - List recordedFieldTypes() { - return recordedFieldTypes; + @Override + public RelDataType createSqlType(final SqlTypeName typeName, final int precision) { + requestedSqlTypeNames.add(typeName); + return super.createSqlType(typeName, precision); } - /** Exposes the protected interning hook so a test can check a type belongs to this factory. */ - RelDataType canonizeType(final RelDataType type) { - return canonize(type); + @Override + public RelDataType createSqlType( + final SqlTypeName typeName, final int precision, final int scale) { + requestedSqlTypeNames.add(typeName); + return super.createSqlType(typeName, precision, scale); + } + + boolean wasUsedForStructType() { + return createStructTypeCalled; + } + + List requestedSqlTypeNames() { + return List.copyOf(requestedSqlTypeNames); } } @@ -89,8 +109,9 @@ void tableListPathUsesProviderTypeFactory() throws SqlParseException { } /** - * The column types are derived through a validator built from the provider, so they are interned - * in the provider's type factory too — not just the enclosing struct type. + * The validator that derives the column types is built from the provider, so the declared column + * types are requested from the provider's type factory — covering the validator path, not just + * the enclosing struct type. */ @Test void columnTypesAreDerivedWithProviderTypeFactory() throws SqlParseException { @@ -99,14 +120,13 @@ void columnTypesAreDerivedWithProviderTypeFactory() throws SqlParseException { SubstraitCreateStatementParser.processCreateStatementsToCatalog(provider, CREATE_STATEMENT); - List fieldTypes = typeFactory.recordedFieldTypes(); - assertEquals(3, fieldTypes.size()); - for (RelDataType fieldType : fieldTypes) { - assertSame( - fieldType, - typeFactory.canonizeType(fieldType), - "column types should be interned in the provider's type factory"); - } + assertTrue( + typeFactory + .requestedSqlTypeNames() + .containsAll(List.of(SqlTypeName.BIGINT, SqlTypeName.VARCHAR, SqlTypeName.DECIMAL)), + "the validator should derive the declared column types through the provider's type " + + "factory, but it only saw " + + typeFactory.requestedSqlTypeNames()); } /** The default entry points keep working off {@link ConverterProvider#DEFAULT}. */