From a29fa2f223e2a20f63c547c465cbeb54d80b8432 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 24 Jul 2026 08:20:46 +0200 Subject: [PATCH 1/4] feat(isthmus): introduce a ConverterProvider builder Add ConverterProvider.builder() and a nested Builder that carries a full Calcite SqlParser.Config alongside extensions, typeFactory, the function converters, typeConverter and executionBehavior. getSqlParserConfig() now returns the stored config, and DEFAULT_SQL_PARSER_CONFIG (the isthmus default: DEFAULT + TO_UPPER + SqlDdlParserImpl.FACTORY + LENIENT) is exposed as the recommended base for customization. Make the builder the sole public construction path: a protected ConverterProvider(Builder) is the stable subclassing seam (build() and the delegating public constructors route through it), and all public constructors are @Deprecated in favour of builder(). DEFAULT is now builder().build(). The two subclasses, DynamicConverterProvider and AutomaticDynamicFunctionMappingConverterProvider, gain a public (Builder) constructor calling super(builder) and deprecate their positional constructors. Callers, tests and examples are migrated to builder()/DEFAULT. --- .../examples/CustomDialectDynamicFnToSql.java | 4 +- .../io/substrait/examples/DynamicFnToSql.java | 4 +- .../io/substrait/examples/ToOptimizedSql.java | 3 +- .../isthmus/cli/IsthmusEntryPoint.java | 16 +- ...namicFunctionMappingConverterProvider.java | 33 +- .../substrait/isthmus/ConverterProvider.java | 282 +++++++++++++++--- .../isthmus/DynamicConverterProvider.java | 28 +- .../io/substrait/isthmus/SqlToSubstrait.java | 12 +- .../io/substrait/isthmus/SubstraitToSql.java | 2 +- .../sql/SubstraitSqlStatementParser.java | 5 +- .../isthmus/AnyValueFunctionTest.java | 2 +- ...icDynamicFunctionMappingRoundtripTest.java | 2 +- .../substrait/isthmus/CustomFunctionTest.java | 15 +- .../io/substrait/isthmus/PlanTestBase.java | 2 +- .../isthmus/RelExtensionRoundtripTest.java | 6 +- .../isthmus/UdfSqlSubstraitTest.java | 4 +- .../UserDefinedLiteralRoundtripTest.java | 15 +- .../PostgreSqlIntegrationTest.java | 2 +- 18 files changed, 355 insertions(+), 82 deletions(-) diff --git a/examples/isthmus-api/src/main/java/io/substrait/examples/CustomDialectDynamicFnToSql.java b/examples/isthmus-api/src/main/java/io/substrait/examples/CustomDialectDynamicFnToSql.java index 61375262c..34e1a4e69 100644 --- a/examples/isthmus-api/src/main/java/io/substrait/examples/CustomDialectDynamicFnToSql.java +++ b/examples/isthmus-api/src/main/java/io/substrait/examples/CustomDialectDynamicFnToSql.java @@ -5,6 +5,7 @@ import io.substrait.expression.Expression; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.ConverterProvider; import io.substrait.isthmus.DynamicConverterProvider; import io.substrait.isthmus.SubstraitToSql; import io.substrait.plan.Plan; @@ -79,7 +80,8 @@ public void run(final String[] args) { // Convert the plan to SQL final SubstraitToSql substraitToSql = - new SubstraitToSql(new DynamicConverterProvider(extensions)); + new SubstraitToSql( + new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions))); System.out.println("\nWith custom SparkSQL SqlDialect::"); substraitToSql.convert(plan, customSqlDialect()).stream().forEachOrdered(System.out::println); diff --git a/examples/isthmus-api/src/main/java/io/substrait/examples/DynamicFnToSql.java b/examples/isthmus-api/src/main/java/io/substrait/examples/DynamicFnToSql.java index 0c3193781..b637c4b61 100644 --- a/examples/isthmus-api/src/main/java/io/substrait/examples/DynamicFnToSql.java +++ b/examples/isthmus-api/src/main/java/io/substrait/examples/DynamicFnToSql.java @@ -5,6 +5,7 @@ import io.substrait.expression.Expression; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.ConverterProvider; import io.substrait.isthmus.DynamicConverterProvider; import io.substrait.isthmus.SubstraitToSql; import io.substrait.plan.Plan; @@ -77,7 +78,8 @@ public void run(final String[] args) { // Convert the plan to SQL final SubstraitToSql substraitToSql = - new SubstraitToSql(new DynamicConverterProvider(extensions)); + new SubstraitToSql( + new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions))); System.out.println("\nWith default DuckDB SqlDialect::"); substraitToSql.convert(plan, SqlDialect.DatabaseProduct.DUCKDB.getDialect()).stream() .forEachOrdered(System.out::println); diff --git a/examples/isthmus-api/src/main/java/io/substrait/examples/ToOptimizedSql.java b/examples/isthmus-api/src/main/java/io/substrait/examples/ToOptimizedSql.java index 9500262c5..a09ac7705 100644 --- a/examples/isthmus-api/src/main/java/io/substrait/examples/ToOptimizedSql.java +++ b/examples/isthmus-api/src/main/java/io/substrait/examples/ToOptimizedSql.java @@ -40,7 +40,8 @@ public void run(String[] args) { final Plan substraitPlan = new ProtoPlanConverter().from(proto); // Configure Isthmus Utilities - final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(new ConverterProvider()); + final SubstraitToCalcite substraitToCalcite = + new SubstraitToCalcite(ConverterProvider.DEFAULT); // Configure Calcite Utilities final SqlDialect sqlDialect = SqlDialect.DatabaseProduct.MYSQL.getDialect(); diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index 22efaaab5..b80bec415 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -15,7 +15,6 @@ import java.util.concurrent.Callable; import org.apache.calcite.avatica.util.Casing; import org.apache.calcite.prepare.Prepare; -import org.apache.calcite.sql.parser.SqlParser; import picocli.CommandLine; import picocli.CommandLine.Command; import picocli.CommandLine.Option; @@ -60,15 +59,6 @@ enum OutputFormat { description = "Calcite's casing policy for unquoted identifiers: ${COMPLETION-CANDIDATES}") private Casing unquotedCasing = Casing.TO_UPPER; - private ConverterProvider converterProvider() { - return new ConverterProvider() { - @Override - public SqlParser.Config getSqlParserConfig() { - return super.getSqlParserConfig().withUnquotedCasing(unquotedCasing); - } - }; - } - /** * Standard Java Main method invoked by the isthmus CLI command. * @@ -96,7 +86,11 @@ public static void main(String... args) { @Override public Integer call() throws Exception { - ConverterProvider provider = converterProvider(); + ConverterProvider provider = + ConverterProvider.builder() + .sqlParserConfig( + ConverterProvider.DEFAULT_SQL_PARSER_CONFIG.withUnquotedCasing(unquotedCasing)) + .build(); // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { SqlExpressionToSubstrait converter = new SqlExpressionToSubstrait(provider); diff --git a/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java index 7d8a3726e..9869625c6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java @@ -47,9 +47,14 @@ public class AutomaticDynamicFunctionMappingConverterProvider extends ConverterP * *

Uses {@link DefaultExtensionCatalog#DEFAULT_COLLECTION} for extensions and {@link * SubstraitTypeSystem#TYPE_FACTORY} for type operations. + * + * @deprecated Use {@link + * #AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builder)} with {@link + * ConverterProvider#builder()} instead. */ + @Deprecated public AutomaticDynamicFunctionMappingConverterProvider() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); + this(ConverterProvider.builder()); } /** @@ -58,10 +63,15 @@ public AutomaticDynamicFunctionMappingConverterProvider() { *

Uses {@link SubstraitTypeSystem#TYPE_FACTORY} for type operations. * * @param extensions the extension collection containing function definitions + * @deprecated Use {@link + * #AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builder)} instead, e.g. + * {@code new AutomaticDynamicFunctionMappingConverterProvider( + * ConverterProvider.builder().extensions(extensions))}. */ + @Deprecated public AutomaticDynamicFunctionMappingConverterProvider( SimpleExtension.ExtensionCollection extensions) { - this(extensions, SubstraitTypeSystem.TYPE_FACTORY); + this(ConverterProvider.builder().extensions(extensions)); } /** @@ -72,10 +82,27 @@ public AutomaticDynamicFunctionMappingConverterProvider( * * @param extensions the extension collection containing function definitions * @param typeFactory the type factory for creating and managing Calcite data types + * @deprecated Use {@link + * #AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builder)} instead, e.g. + * {@code new AutomaticDynamicFunctionMappingConverterProvider( + * ConverterProvider.builder().extensions(extensions).typeFactory(typeFactory))}. */ + @Deprecated public AutomaticDynamicFunctionMappingConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - super(extensions, typeFactory); + this(ConverterProvider.builder().extensions(extensions).typeFactory(typeFactory)); + } + + /** + * Creates a new provider from a {@link ConverterProvider.Builder}, seeding base state from the + * builder and then installing the automatically generated dynamic function mappings and operator + * table. This is the seam through which the other constructors — and subclasses, via {@code + * super(builder)} — route. + * + * @param builder the builder carrying the configured components + */ + public AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builder builder) { + super(builder); List dynamicScalarOperators = getDynamicScalarOperators(); this.scalarFunctionConverter = createScalarFunctionConverter(dynamicScalarOperators); diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index f8b4b130e..b9e04a744 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -40,21 +40,52 @@ * *

It is consumed by all conversion classes as their primary source of configuration. * - *

The no argument constructor {@link #ConverterProvider()} provides reasonable system defaults. + *

{@link #DEFAULT} is a shared instance configured with reasonable system defaults, equivalent + * to {@code builder().build()}. * - *

Other constructors allow for further customization of conversion behaviours. + *

For customized conversion behaviour — including supplying a full Calcite {@link + * SqlParser.Config} for SQL parsing — use the {@link #builder()}. * *

More in-depth customization can be achieved by extending this class, as is done in {@link * DynamicConverterProvider}. */ public class ConverterProvider { + /** + * The default Calcite {@link SqlParser.Config} used by isthmus: {@link SqlParser.Config#DEFAULT} + * with {@link Casing#TO_UPPER} unquoted-identifier casing, the {@link SqlDdlParserImpl} parser + * factory (so {@code CREATE TABLE} statements parse), and {@link SqlConformanceEnum#LENIENT} + * conformance. + * + *

This is the recommended starting point for a customized parser configuration: derive from it + * with Calcite's {@code withXxx} methods and pass the result to {@link + * Builder#sqlParserConfig(SqlParser.Config)}, e.g. {@code + * DEFAULT_SQL_PARSER_CONFIG.withUnquotedCasing(Casing.UNCHANGED)}. + */ + public static final SqlParser.Config DEFAULT_SQL_PARSER_CONFIG = + SqlParser.Config.DEFAULT + .withUnquotedCasing(Casing.TO_UPPER) + .withParserFactory(SqlDdlParserImpl.FACTORY) + .withConformance(SqlConformanceEnum.LENIENT); + + /** + * A shared default {@link ConverterProvider} instance using all system defaults. Equivalent to + * {@code builder().build()} but avoids redundant construction at every call site. + * + *

This instance is safe to share because {@link ConverterProvider} is effectively immutable + * after construction — all fields are set only in constructors. + */ + public static final ConverterProvider DEFAULT = builder().build(); + /** The Calcite type factory used for creating and managing data types. */ protected RelDataTypeFactory typeFactory; /** The collection of Substrait extensions (functions and types) available for conversion. */ protected final SimpleExtension.ExtensionCollection extensions; + /** The Calcite SQL parser configuration, controlling parsing behaviour like identifier casing. */ + protected final SqlParser.Config sqlParserConfig; + /** Converter for Substrait scalar functions. */ protected ScalarFunctionConverter scalarFunctionConverter; @@ -70,30 +101,27 @@ public class ConverterProvider { /** The execution behavior configuration for plans created by this converter. */ protected final Plan.ExecutionBehavior executionBehavior; - /** - * A shared default {@link ConverterProvider} instance using all system defaults. Equivalent to - * {@code new ConverterProvider()} but avoids redundant construction at every call site. - * - *

This instance is safe to share because {@link ConverterProvider} is effectively immutable - * after construction — all fields are set only in constructors. - */ - public static final ConverterProvider DEFAULT = new ConverterProvider(); - /** * Creates a ConverterProvider with default extension collection and type factory. Uses {@link * DefaultExtensionCatalog#DEFAULT_COLLECTION} and {@link SubstraitTypeSystem#TYPE_FACTORY}. + * + * @deprecated Use {@link #builder()} (or the shared {@link #DEFAULT} instance) instead. */ + @Deprecated public ConverterProvider() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); + this(builder()); } /** * Creates a ConverterProvider with the specified extension collection and default type factory. * * @param extensions the Substrait extension collection to use + * @deprecated Use {@link #builder()} instead, e.g. {@code + * builder().extensions(extensions).build()}. */ + @Deprecated public ConverterProvider(SimpleExtension.ExtensionCollection extensions) { - this(extensions, SubstraitTypeSystem.TYPE_FACTORY); + this(builder().extensions(extensions)); } /** @@ -101,16 +129,13 @@ public ConverterProvider(SimpleExtension.ExtensionCollection extensions) { * * @param extensions the Substrait extension collection to use * @param typeFactory the Calcite type factory to use + * @deprecated Use {@link #builder()} instead, e.g. {@code + * builder().extensions(extensions).typeFactory(typeFactory).build()}. */ + @Deprecated public ConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - this( - typeFactory, - extensions, - new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), - new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory), - new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), - TypeConverter.DEFAULT); + this(builder().extensions(extensions).typeFactory(typeFactory)); } /** @@ -122,7 +147,10 @@ public ConverterProvider( * @param afc the aggregate function converter to use * @param wfc the window function converter to use * @param tc the type converter to use + * @deprecated Use {@link #builder()} instead; the growing set of components is more readably + * configured through the builder than through this positional constructor. */ + @Deprecated public ConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions, @@ -130,7 +158,14 @@ public ConverterProvider( AggregateFunctionConverter afc, WindowFunctionConverter wfc, TypeConverter tc) { - this(typeFactory, extensions, sfc, afc, wfc, tc, createDefaultExecutionBehavior()); + this( + builder() + .typeFactory(typeFactory) + .extensions(extensions) + .scalarFunctionConverter(sfc) + .aggregateFunctionConverter(afc) + .windowFunctionConverter(wfc) + .typeConverter(tc)); } /** @@ -143,7 +178,10 @@ public ConverterProvider( * @param wfc the window function converter to use * @param tc the type converter to use * @param executionBehavior the execution behavior to use for plans + * @deprecated Use {@link #builder()} instead; the growing set of components is more readably + * configured through the builder than through this positional constructor. */ + @Deprecated public ConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions, @@ -152,13 +190,50 @@ public ConverterProvider( WindowFunctionConverter wfc, TypeConverter tc, Plan.ExecutionBehavior executionBehavior) { - this.typeFactory = typeFactory; - this.extensions = extensions; - this.scalarFunctionConverter = sfc; - this.aggregateFunctionConverter = afc; - this.windowFunctionConverter = wfc; - this.typeConverter = tc; - this.executionBehavior = executionBehavior; + this( + builder() + .typeFactory(typeFactory) + .extensions(extensions) + .scalarFunctionConverter(sfc) + .aggregateFunctionConverter(afc) + .windowFunctionConverter(wfc) + .typeConverter(tc) + .executionBehavior(executionBehavior)); + } + + /** + * Master constructor and the sole subclassing seam: derives any unset function converters from + * the configured extensions and type factory, then assigns all components. {@link + * Builder#build()} and every delegating public constructor route here, and subclasses (e.g. + * {@link DynamicConverterProvider}) invoke it via {@code super(builder)}. + * + *

Taking the {@link Builder} rather than a positional argument list keeps this seam stable as + * new components are added: a new field is a change to the builder and this constructor only, not + * to every subclass's {@code super(...)} call. + * + * @param builder the builder carrying the configured components + */ + protected ConverterProvider(Builder builder) { + this.typeFactory = builder.typeFactory; + this.extensions = builder.extensions; + this.scalarFunctionConverter = + builder.scalarFunctionConverter != null + ? builder.scalarFunctionConverter + : new ScalarFunctionConverter( + builder.extensions.scalarFunctions(), builder.typeFactory); + this.aggregateFunctionConverter = + builder.aggregateFunctionConverter != null + ? builder.aggregateFunctionConverter + : new AggregateFunctionConverter( + builder.extensions.aggregateFunctions(), builder.typeFactory); + this.windowFunctionConverter = + builder.windowFunctionConverter != null + ? builder.windowFunctionConverter + : new WindowFunctionConverter( + builder.extensions.windowFunctions(), builder.typeFactory); + this.typeConverter = builder.typeConverter; + this.executionBehavior = builder.executionBehavior; + this.sqlParserConfig = builder.sqlParserConfig; } /** @@ -178,13 +253,14 @@ private static Plan.ExecutionBehavior createDefaultExecutionBehavior() { * {@link SqlParser.Config} is a Calcite class which controls SQL parsing behaviour like * identifier casing. * + *

Defaults to {@link #DEFAULT_SQL_PARSER_CONFIG}. Provide a custom configuration via {@link + * Builder#sqlParserConfig(SqlParser.Config)}, or override this method in a subclass for fully + * dynamic behaviour. + * * @return the SQL parser configuration */ public SqlParser.Config getSqlParserConfig() { - return SqlParser.Config.DEFAULT - .withUnquotedCasing(Casing.TO_UPPER) - .withParserFactory(SqlDdlParserImpl.FACTORY) - .withConformance(SqlConformanceEnum.LENIENT); + return sqlParserConfig; } /** @@ -424,12 +500,150 @@ public TypeConverter getTypeConverter() { * *

The default execution behavior uses {@link * Plan.ExecutionBehavior.VariableEvaluationMode#PER_PLAN}, which evaluates variables once per - * plan execution. This can be customized by providing a different execution behavior through the - * constructor. + * plan execution. This can be customized via {@link + * Builder#executionBehavior(Plan.ExecutionBehavior)}. * * @return the execution behavior to use when creating plans */ public Plan.ExecutionBehavior getExecutionBehavior() { return executionBehavior; } + + /** + * Creates a new {@link Builder} for configuring a {@link ConverterProvider}. + * + *

The builder starts from reasonable system defaults (the same ones behind {@link #DEFAULT}) + * and lets callers override individual components — most notably the Calcite {@link + * SqlParser.Config} used for SQL parsing, via {@link Builder#sqlParserConfig(SqlParser.Config)}. + * + * @return a new builder + */ + public static Builder builder() { + return new Builder(); + } + + /** + * Fluent builder for {@link ConverterProvider}. + * + *

Unset components fall back to reasonable system defaults. The scalar, aggregate and window + * function converters, if not set explicitly, are derived from the configured {@link + * #extensions(SimpleExtension.ExtensionCollection) extensions} and {@link + * #typeFactory(RelDataTypeFactory) type factory} when {@link #build()} is called. + */ + public static class Builder { + private SimpleExtension.ExtensionCollection extensions = + DefaultExtensionCatalog.DEFAULT_COLLECTION; + private RelDataTypeFactory typeFactory = SubstraitTypeSystem.TYPE_FACTORY; + private ScalarFunctionConverter scalarFunctionConverter; + private AggregateFunctionConverter aggregateFunctionConverter; + private WindowFunctionConverter windowFunctionConverter; + private TypeConverter typeConverter = TypeConverter.DEFAULT; + private Plan.ExecutionBehavior executionBehavior = createDefaultExecutionBehavior(); + private SqlParser.Config sqlParserConfig = DEFAULT_SQL_PARSER_CONFIG; + + /** + * Sets the Substrait extension collection to use. + * + * @param extensions the extension collection + * @return this builder + */ + public Builder extensions(SimpleExtension.ExtensionCollection extensions) { + this.extensions = extensions; + return this; + } + + /** + * Sets the Calcite type factory to use. + * + * @param typeFactory the type factory + * @return this builder + */ + public Builder typeFactory(RelDataTypeFactory typeFactory) { + this.typeFactory = typeFactory; + return this; + } + + /** + * Sets the scalar function converter. When left unset, it is derived from the configured + * extensions and type factory. + * + * @param scalarFunctionConverter the scalar function converter + * @return this builder + */ + public Builder scalarFunctionConverter(ScalarFunctionConverter scalarFunctionConverter) { + this.scalarFunctionConverter = scalarFunctionConverter; + return this; + } + + /** + * Sets the aggregate function converter. When left unset, it is derived from the configured + * extensions and type factory. + * + * @param aggregateFunctionConverter the aggregate function converter + * @return this builder + */ + public Builder aggregateFunctionConverter( + AggregateFunctionConverter aggregateFunctionConverter) { + this.aggregateFunctionConverter = aggregateFunctionConverter; + return this; + } + + /** + * Sets the window function converter. When left unset, it is derived from the configured + * extensions and type factory. + * + * @param windowFunctionConverter the window function converter + * @return this builder + */ + public Builder windowFunctionConverter(WindowFunctionConverter windowFunctionConverter) { + this.windowFunctionConverter = windowFunctionConverter; + return this; + } + + /** + * Sets the type converter. + * + * @param typeConverter the type converter + * @return this builder + */ + public Builder typeConverter(TypeConverter typeConverter) { + this.typeConverter = typeConverter; + return this; + } + + /** + * Sets the execution behavior for plans created by the resulting converter. + * + * @param executionBehavior the execution behavior + * @return this builder + */ + public Builder executionBehavior(Plan.ExecutionBehavior executionBehavior) { + this.executionBehavior = executionBehavior; + return this; + } + + /** + * Sets the full Calcite {@link SqlParser.Config} used for SQL parsing, replacing the default. + * + *

Use {@link ConverterProvider#DEFAULT_SQL_PARSER_CONFIG} as a starting point to retain + * isthmus' DDL parser factory and conformance while overriding individual settings. + * + * @param sqlParserConfig the parser configuration + * @return this builder + */ + public Builder sqlParserConfig(SqlParser.Config sqlParserConfig) { + this.sqlParserConfig = sqlParserConfig; + return this; + } + + /** + * Builds a {@link ConverterProvider} from the configured components, deriving any unset + * function converters from the configured extensions and type factory. + * + * @return a new {@link ConverterProvider} + */ + public ConverterProvider build() { + return new ConverterProvider(this); + } + } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 39d47b504..abc0288f3 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -42,9 +42,13 @@ public class DynamicConverterProvider extends ConverterProvider { * *

Uses {@link DefaultExtensionCatalog#DEFAULT_COLLECTION} for extensions and {@link * SubstraitTypeSystem#TYPE_FACTORY} for type operations. + * + * @deprecated Use {@link #DynamicConverterProvider(ConverterProvider.Builder)} with {@link + * ConverterProvider#builder()} instead. */ + @Deprecated public DynamicConverterProvider() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); + this(ConverterProvider.builder()); } /** @@ -53,9 +57,12 @@ public DynamicConverterProvider() { *

Uses {@link SubstraitTypeSystem#TYPE_FACTORY} for type operations. * * @param extensions the collection of Substrait extensions to use for function mappings + * @deprecated Use {@link #DynamicConverterProvider(ConverterProvider.Builder)} instead, e.g. + * {@code new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions))}. */ + @Deprecated public DynamicConverterProvider(SimpleExtension.ExtensionCollection extensions) { - this(extensions, SubstraitTypeSystem.TYPE_FACTORY); + this(ConverterProvider.builder().extensions(extensions)); } /** @@ -67,10 +74,25 @@ public DynamicConverterProvider(SimpleExtension.ExtensionCollection extensions) * * @param extensions the collection of Substrait extensions to use for function mappings * @param typeFactory the factory to use for creating and managing relational data types + * @deprecated Use {@link #DynamicConverterProvider(ConverterProvider.Builder)} instead, e.g. + * {@code new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions) + * .typeFactory(typeFactory))}. */ + @Deprecated public DynamicConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - super(extensions, typeFactory); + this(ConverterProvider.builder().extensions(extensions).typeFactory(typeFactory)); + } + + /** + * Creates a new DynamicConverterProvider from a {@link ConverterProvider.Builder}, seeding base + * state from the builder and then installing the converter that handles dynamic extension + * functions. This is the seam through which subclasses route via {@code super(builder)}. + * + * @param builder the builder carrying the configured components + */ + public DynamicConverterProvider(ConverterProvider.Builder builder) { + super(builder); this.scalarFunctionConverter = createScalarFunctionConverter(); this.operatorTable = buildSqlOperatorTable(); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index 284572de9..6431c4951 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -56,8 +56,9 @@ public Plan convert(final String sqlStatements, final Prepare.CatalogReader cata * Converts one or more SQL statements into a Substrait {@link Plan}. * *

The {@code sqlDialect} parameter was previously used to influence identifier casing during - * parsing. This is now controlled by the {@link ConverterProvider} supplied to this converter; to - * customise it, subclass {@link ConverterProvider} and override {@link + * parsing. This is now configurable via {@link ConverterProvider#builder()} (for example {@code + * ConverterProvider.builder().unquotedCasing(...)}), or for fully custom parser behaviour, by + * subclassing {@link ConverterProvider} and overriding {@link * ConverterProvider#getSqlParserConfig()}. * * @param sqlStatements a string containing one more SQL statements @@ -67,9 +68,10 @@ public Plan convert(final String sqlStatements, final Prepare.CatalogReader cata * @return the Substrait {@link Plan} * @throws SqlParseException if there is an error while parsing the SQL statements * @deprecated Prefer constructing {@link SqlToSubstrait} with a {@link ConverterProvider} - * configured for the desired casing and calling {@link #convert(String, - * Prepare.CatalogReader)}. For fully custom parser behaviour, subclass {@link - * ConverterProvider} and override {@link ConverterProvider#getSqlParserConfig()}. + * configured for the desired casing (via {@link ConverterProvider#builder()}) and calling + * {@link #convert(String, Prepare.CatalogReader)}. For fully custom parser behaviour, + * subclass {@link ConverterProvider} and override {@link + * ConverterProvider#getSqlParserConfig()}. */ @Deprecated public Plan convert( diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java index 690bd5774..ac07d8d65 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java @@ -23,7 +23,7 @@ public class SubstraitToSql extends SqlConverterBase { /** Creates a SubstraitToSql converter with default configuration and extensions. */ public SubstraitToSql() { - this(new ConverterProvider()); + this(ConverterProvider.DEFAULT); } /** diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java index a7cd777e6..1bb73763a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java @@ -27,8 +27,9 @@ public static List parseStatements(String sqlStatements) throws SqlPars * Parse one or more SQL statements to a list of {@link SqlNode}s, using the parser settings from * the given {@link ConverterProvider}. * - *

To use a custom parser configuration, subclass {@link ConverterProvider} and override {@link - * ConverterProvider#getSqlParserConfig()}. + *

To use a custom parser configuration, build the {@link ConverterProvider} via {@link + * ConverterProvider#builder()} and its {@code sqlParserConfig(...)}; for fully dynamic behaviour, + * subclass {@link ConverterProvider} and override {@link ConverterProvider#getSqlParserConfig()}. * * @param sqlStatements a string containing one or more SQL statements * @param converterProvider the converter provider whose parser config controls identifier casing diff --git a/isthmus/src/test/java/io/substrait/isthmus/AnyValueFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/AnyValueFunctionTest.java index 4b167050b..075166b19 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/AnyValueFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/AnyValueFunctionTest.java @@ -5,7 +5,7 @@ class AnyValueFunctionTest extends PlanTestBase { AnyValueFunctionTest() { - super(new AutomaticDynamicFunctionMappingConverterProvider()); + super(new AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.builder())); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java index 03f0c8f0f..67421f4f4 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java @@ -24,7 +24,7 @@ class AutomaticDynamicFunctionMappingRoundtripTest extends PlanTestBase { AutomaticDynamicFunctionMappingRoundtripTest() { - super(new AutomaticDynamicFunctionMappingConverterProvider()); + super(new AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.builder())); } /** diff --git a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java index b5a7317b9..e2d60669a 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java @@ -257,13 +257,14 @@ public RelDataType toCalcite(Type.UserDefined type) { CustomFunctionTest() { super( - new ConverterProvider( - SubstraitTypeSystem.TYPE_FACTORY, - CUSTOM_EXTENSIONS, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter)); + ConverterProvider.builder() + .typeFactory(SubstraitTypeSystem.TYPE_FACTORY) + .extensions(CUSTOM_EXTENSIONS) + .scalarFunctionConverter(scalarFunctionConverter) + .aggregateFunctionConverter(aggregateFunctionConverter) + .windowFunctionConverter(windowFunctionConverter) + .typeConverter(typeConverter) + .build()); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index 7686f5e0b..c560b29b7 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -70,7 +70,7 @@ public class PlanTestBase { PlanTestBase.schemaToCatalog("tpcds", TPCDS_SCHEMA); protected PlanTestBase() { - this(new ConverterProvider()); + this(ConverterProvider.DEFAULT); } protected PlanTestBase(ConverterProvider converterProvider) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java index 5ec26b92b..0d696fe40 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java @@ -82,7 +82,11 @@ void roundtrip(Rel pojo1) { // Calcite -> Substrait POJO 3 Rel pojo3 = - (new CustomSubstraitRelVisitor(new ConverterProvider(extensions, typeFactory))) + (new CustomSubstraitRelVisitor( + ConverterProvider.builder() + .extensions(extensions) + .typeFactory(typeFactory) + .build())) .apply(calcite); assertEquals(pojo1, pojo3); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java index cbdbe2fa3..bebcd1021 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java @@ -12,7 +12,9 @@ class UdfSqlSubstraitTest extends PlanTestBase { private static final String CUSTOM_FUNCTION_PATH = "/extensions/scalar_functions_custom.yaml"; UdfSqlSubstraitTest() { - super(new DynamicConverterProvider(loadExtensions(List.of(CUSTOM_FUNCTION_PATH)))); + super( + new DynamicConverterProvider( + ConverterProvider.builder().extensions(loadExtensions(List.of(CUSTOM_FUNCTION_PATH))))); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/UserDefinedLiteralRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/UserDefinedLiteralRoundtripTest.java index f97616ab6..4678b4b16 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UserDefinedLiteralRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UserDefinedLiteralRoundtripTest.java @@ -140,13 +140,14 @@ class UserDefinedLiteralRoundtripTest extends PlanTestBase { UserDefinedLiteralRoundtripTest() { super( - new ConverterProvider( - SubstraitTypeSystem.TYPE_FACTORY, - NESTED_TYPES_EXTENSIONS, - SCALAR_FUNCTION_CONVERTER, - AGGREGATE_FUNCTION_CONVERTER, - WINDOW_FUNCTION_CONVERTER, - TYPE_CONVERTER)); + ConverterProvider.builder() + .typeFactory(SubstraitTypeSystem.TYPE_FACTORY) + .extensions(NESTED_TYPES_EXTENSIONS) + .scalarFunctionConverter(SCALAR_FUNCTION_CONVERTER) + .aggregateFunctionConverter(AGGREGATE_FUNCTION_CONVERTER) + .windowFunctionConverter(WINDOW_FUNCTION_CONVERTER) + .typeConverter(TYPE_CONVERTER) + .build()); } private void assertRoundTrip(Expression.UserDefinedLiteral literal) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/integration/PostgreSqlIntegrationTest.java b/isthmus/src/test/java/io/substrait/isthmus/integration/PostgreSqlIntegrationTest.java index 8429cf834..50bd6ecc2 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/integration/PostgreSqlIntegrationTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/integration/PostgreSqlIntegrationTest.java @@ -101,7 +101,7 @@ void testTpcH(final int queryNo) final SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(); final Plan plan = sqlToSubstrait.convert(inputSql, TPCH_CATALOG); - final ConverterProvider provider = new ConverterProvider(extensions); + final ConverterProvider provider = ConverterProvider.builder().extensions(extensions).build(); final SubstraitToSql substraitToSql = new SubstraitToSql(provider); final String generatedSql = substraitToSql.convert(plan, PostgresqlSqlDialect.DEFAULT).get(0); From defa554c9fd7748a8ef6b92d7f9c61e12abdde83 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Sat, 25 Jul 2026 08:35:10 +0200 Subject: [PATCH 2/4] refactor(isthmus): source CREATE-statement catalog config from the ConverterProvider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processCreateStatementsToCatalog now builds the CalciteCatalogReader with the injected ConverterProvider's type factory (getTypeFactory()) and connection config (getCalciteConnectionConfig()) rather than the hardcoded SubstraitTypeSystem.TYPE_FACTORY / SqlConverterBase.CONNECTION_CONFIG, so a custom provider's type factory and connection config are honored end-to-end. The no-arg overload now delegates to the ConverterProvider overload (with DEFAULT), removing the duplicated reader construction. Behavior is unchanged for the default provider — same type factory and connection config. --- .../isthmus/sql/SubstraitCreateStatementParser.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) 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 3d62e2726..a6b52cd21 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java @@ -142,14 +142,7 @@ public static CalciteCatalogReader processCreateStatementsToCatalog( */ public static CalciteCatalogReader processCreateStatementsToCatalog( @NonNull final String... createStatements) throws SqlParseException { - final CalciteSchema rootSchema = - processCreateStatementsToSchema(ConverterProvider.DEFAULT, createStatements); - final List defaultSchema = Collections.emptyList(); - return new CalciteCatalogReader( - rootSchema, - defaultSchema, - SubstraitTypeSystem.TYPE_FACTORY, - SqlConverterBase.CONNECTION_CONFIG); + return processCreateStatementsToCatalog(ConverterProvider.DEFAULT, createStatements); } /** @@ -173,8 +166,8 @@ public static CalciteCatalogReader processCreateStatementsToCatalog( return new CalciteCatalogReader( rootSchema, defaultSchema, - SubstraitTypeSystem.TYPE_FACTORY, - SqlConverterBase.CONNECTION_CONFIG); + converterProvider.getTypeFactory(), + converterProvider.getCalciteConnectionConfig()); } /** From 6fb27d82a530caf265fb205273e843a196653929 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Sat, 25 Jul 2026 09:03:04 +0200 Subject: [PATCH 3/4] refactor(isthmus): honor the ConverterProvider's configured TypeConverter Two spots still used TypeConverter.DEFAULT instead of the provider's: - ConverterProvider(Builder) built the scalar/aggregate/window function converters via their 2-arg constructors, which fall back to TypeConverter.DEFAULT, so a TypeConverter set through Builder.typeConverter(...) was ignored by those converters. Pass builder.typeConverter through. - SqlExpressionToSubstrait.toNamedStruct converted column types with TypeConverter.DEFAULT; use the injected provider's getTypeConverter(). Behavior is unchanged for the default provider, whose type converter is TypeConverter.DEFAULT. --- .../io/substrait/isthmus/ConverterProvider.java | 15 ++++++++++++--- .../isthmus/SqlExpressionToSubstrait.java | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index b9e04a744..ac1f69977 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -220,17 +220,26 @@ protected ConverterProvider(Builder builder) { builder.scalarFunctionConverter != null ? builder.scalarFunctionConverter : new ScalarFunctionConverter( - builder.extensions.scalarFunctions(), builder.typeFactory); + builder.extensions.scalarFunctions(), + List.of(), + builder.typeFactory, + builder.typeConverter); this.aggregateFunctionConverter = builder.aggregateFunctionConverter != null ? builder.aggregateFunctionConverter : new AggregateFunctionConverter( - builder.extensions.aggregateFunctions(), builder.typeFactory); + builder.extensions.aggregateFunctions(), + List.of(), + builder.typeFactory, + builder.typeConverter); this.windowFunctionConverter = builder.windowFunctionConverter != null ? builder.windowFunctionConverter : new WindowFunctionConverter( - builder.extensions.windowFunctions(), builder.typeFactory); + builder.extensions.windowFunctions(), + List.of(), + builder.typeFactory, + builder.typeConverter); this.typeConverter = builder.typeConverter; this.executionBehavior = builder.executionBehavior; this.sqlParserConfig = builder.sqlParserConfig; diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java index af5b6c855..2d258a27e 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java @@ -242,7 +242,7 @@ private NamedStruct toNamedStruct(Map nameToTypeMap) { String k = entry.getKey(); RelDataType v = entry.getValue(); names.add(k); - types.add(TypeConverter.DEFAULT.toSubstrait(v)); + types.add(converterProvider.getTypeConverter().toSubstrait(v)); } return NamedStruct.of(names, Type.Struct.builder().fields(types).nullable(false).build()); } From ff3176da9446c7d83b66adbde1523dd34c0e1636 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 31 Jul 2026 10:23:20 +0200 Subject: [PATCH 4/4] refactor(isthmus): make unset builder function converters explicit Model the three deferred function converters on ConverterProvider.Builder as Optional rather than nullable fields, and expose them through getters. "Unset" becomes explicit at the type level, and the primary constructor derives them with orElseGet instead of a null check. With the setting now observable, the two providers that derive their own function converters reject a builder that configures one instead of silently discarding it: - DynamicConverterProvider rejects a configured scalarFunctionConverter. - AutomaticDynamicFunctionMappingConverterProvider rejects a configured scalar, aggregate or window function converter. Both validate inside the super(...) argument, since statements before super() are not available on JDK 17. Also group the directly-set builder fields ahead of the derived ones, and trim Javadoc that explained why a builder is used or pointed at subclassing where the builder now suffices. --- ...namicFunctionMappingConverterProvider.java | 34 +++- .../substrait/isthmus/ConverterProvider.java | 160 ++++++++++-------- .../isthmus/DynamicConverterProvider.java | 25 ++- .../sql/SubstraitSqlStatementParser.java | 3 +- .../isthmus/ConverterProviderBuilderTest.java | 116 +++++++++++++ 5 files changed, 265 insertions(+), 73 deletions(-) create mode 100644 isthmus/src/test/java/io/substrait/isthmus/ConverterProviderBuilderTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java index 9869625c6..146b90612 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingConverterProvider.java @@ -11,6 +11,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Optional; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.calcite.rel.type.RelDataTypeFactory; @@ -96,13 +97,14 @@ public AutomaticDynamicFunctionMappingConverterProvider( /** * Creates a new provider from a {@link ConverterProvider.Builder}, seeding base state from the * builder and then installing the automatically generated dynamic function mappings and operator - * table. This is the seam through which the other constructors — and subclasses, via {@code - * super(builder)} — route. + * table. * * @param builder the builder carrying the configured components + * @throws IllegalArgumentException if the builder configures a scalar, aggregate or window + * function converter, all of which this provider derives itself */ public AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builder builder) { - super(builder); + super(requireDerivedFunctionConverters(builder)); List dynamicScalarOperators = getDynamicScalarOperators(); this.scalarFunctionConverter = createScalarFunctionConverter(dynamicScalarOperators); @@ -120,6 +122,32 @@ public AutomaticDynamicFunctionMappingConverterProvider(ConverterProvider.Builde this.operatorTable = buildOperatorTable(allOperators); } + /** + * Rejects a builder that configures any function converter: this provider derives all three from + * the unmapped extension functions, so a configured one would be silently discarded. + * + * @param builder the builder to check + * @return the given builder + * @throws IllegalArgumentException if the builder configures a function converter + */ + private static ConverterProvider.Builder requireDerivedFunctionConverters( + ConverterProvider.Builder builder) { + requireUnset(builder.getScalarFunctionConverter(), "scalarFunctionConverter"); + requireUnset(builder.getAggregateFunctionConverter(), "aggregateFunctionConverter"); + requireUnset(builder.getWindowFunctionConverter(), "windowFunctionConverter"); + return builder; + } + + private static void requireUnset(Optional functionConverter, String setterName) { + if (functionConverter.isPresent()) { + throw new IllegalArgumentException( + "AutomaticDynamicFunctionMappingConverterProvider derives its own function converters " + + "from the unmapped extension functions; remove the ConverterProvider.Builder." + + setterName + + "(...) setting"); + } + } + /** * Returns the SQL operator table containing both base and dynamically mapped operators. * diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index ac1f69977..f6e126358 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -17,6 +17,7 @@ import io.substrait.relation.Rel; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.function.Function; import org.apache.calcite.avatica.util.Casing; import org.apache.calcite.config.CalciteConnectionConfig; @@ -52,7 +53,7 @@ public class ConverterProvider { /** - * The default Calcite {@link SqlParser.Config} used by isthmus: {@link SqlParser.Config#DEFAULT} + * The default Calcite {@link SqlParser.Config} used by Isthmus: {@link SqlParser.Config#DEFAULT} * with {@link Casing#TO_UPPER} unquoted-identifier casing, the {@link SqlDdlParserImpl} parser * factory (so {@code CREATE TABLE} statements parse), and {@link SqlConformanceEnum#LENIENT} * conformance. @@ -202,47 +203,42 @@ public ConverterProvider( } /** - * Master constructor and the sole subclassing seam: derives any unset function converters from - * the configured extensions and type factory, then assigns all components. {@link - * Builder#build()} and every delegating public constructor route here, and subclasses (e.g. - * {@link DynamicConverterProvider}) invoke it via {@code super(builder)}. - * - *

Taking the {@link Builder} rather than a positional argument list keeps this seam stable as - * new components are added: a new field is a change to the builder and this constructor only, not - * to every subclass's {@code super(...)} call. + * Primary constructor for ConverterProvider. The {@link Builder} passed in can be used to further + * customize behaviours. * * @param builder the builder carrying the configured components */ protected ConverterProvider(Builder builder) { this.typeFactory = builder.typeFactory; this.extensions = builder.extensions; - this.scalarFunctionConverter = - builder.scalarFunctionConverter != null - ? builder.scalarFunctionConverter - : new ScalarFunctionConverter( - builder.extensions.scalarFunctions(), - List.of(), - builder.typeFactory, - builder.typeConverter); - this.aggregateFunctionConverter = - builder.aggregateFunctionConverter != null - ? builder.aggregateFunctionConverter - : new AggregateFunctionConverter( - builder.extensions.aggregateFunctions(), - List.of(), - builder.typeFactory, - builder.typeConverter); - this.windowFunctionConverter = - builder.windowFunctionConverter != null - ? builder.windowFunctionConverter - : new WindowFunctionConverter( - builder.extensions.windowFunctions(), - List.of(), - builder.typeFactory, - builder.typeConverter); this.typeConverter = builder.typeConverter; this.executionBehavior = builder.executionBehavior; this.sqlParserConfig = builder.sqlParserConfig; + + this.scalarFunctionConverter = + builder.scalarFunctionConverter.orElseGet( + () -> + new ScalarFunctionConverter( + this.extensions.scalarFunctions(), + List.of(), + this.typeFactory, + this.typeConverter)); + this.aggregateFunctionConverter = + builder.aggregateFunctionConverter.orElseGet( + () -> + new AggregateFunctionConverter( + this.extensions.aggregateFunctions(), + List.of(), + this.typeFactory, + this.typeConverter)); + this.windowFunctionConverter = + builder.windowFunctionConverter.orElseGet( + () -> + new WindowFunctionConverter( + this.extensions.windowFunctions(), + List.of(), + this.typeFactory, + this.typeConverter)); } /** @@ -543,13 +539,15 @@ public static class Builder { private SimpleExtension.ExtensionCollection extensions = DefaultExtensionCatalog.DEFAULT_COLLECTION; private RelDataTypeFactory typeFactory = SubstraitTypeSystem.TYPE_FACTORY; - private ScalarFunctionConverter scalarFunctionConverter; - private AggregateFunctionConverter aggregateFunctionConverter; - private WindowFunctionConverter windowFunctionConverter; private TypeConverter typeConverter = TypeConverter.DEFAULT; private Plan.ExecutionBehavior executionBehavior = createDefaultExecutionBehavior(); private SqlParser.Config sqlParserConfig = DEFAULT_SQL_PARSER_CONFIG; + // Derived from the extensions and type factory at build time when left unset. + private Optional scalarFunctionConverter = Optional.empty(); + private Optional aggregateFunctionConverter = Optional.empty(); + private Optional windowFunctionConverter = Optional.empty(); + /** * Sets the Substrait extension collection to use. * @@ -573,78 +571,108 @@ public Builder typeFactory(RelDataTypeFactory typeFactory) { } /** - * Sets the scalar function converter. When left unset, it is derived from the configured - * extensions and type factory. + * Sets the type converter. * - * @param scalarFunctionConverter the scalar function converter + * @param typeConverter the type converter * @return this builder */ - public Builder scalarFunctionConverter(ScalarFunctionConverter scalarFunctionConverter) { - this.scalarFunctionConverter = scalarFunctionConverter; + public Builder typeConverter(TypeConverter typeConverter) { + this.typeConverter = typeConverter; return this; } /** - * Sets the aggregate function converter. When left unset, it is derived from the configured - * extensions and type factory. + * Sets the execution behavior for plans created by the resulting converter. * - * @param aggregateFunctionConverter the aggregate function converter + * @param executionBehavior the execution behavior * @return this builder */ - public Builder aggregateFunctionConverter( - AggregateFunctionConverter aggregateFunctionConverter) { - this.aggregateFunctionConverter = aggregateFunctionConverter; + public Builder executionBehavior(Plan.ExecutionBehavior executionBehavior) { + this.executionBehavior = executionBehavior; return this; } /** - * Sets the window function converter. When left unset, it is derived from the configured - * extensions and type factory. + * Sets the full Calcite {@link SqlParser.Config} used for SQL parsing, replacing the default. * - * @param windowFunctionConverter the window function converter + *

Use {@link ConverterProvider#DEFAULT_SQL_PARSER_CONFIG} as a starting point to retain + * Isthmus' DDL parser factory and conformance while overriding individual settings. + * + * @param sqlParserConfig the parser configuration * @return this builder */ - public Builder windowFunctionConverter(WindowFunctionConverter windowFunctionConverter) { - this.windowFunctionConverter = windowFunctionConverter; + public Builder sqlParserConfig(SqlParser.Config sqlParserConfig) { + this.sqlParserConfig = sqlParserConfig; return this; } /** - * Sets the type converter. + * Sets the scalar function converter. When left unset, it is derived from the configured + * extensions and type factory. * - * @param typeConverter the type converter + * @param scalarFunctionConverter the scalar function converter * @return this builder */ - public Builder typeConverter(TypeConverter typeConverter) { - this.typeConverter = typeConverter; + public Builder scalarFunctionConverter(ScalarFunctionConverter scalarFunctionConverter) { + this.scalarFunctionConverter = Optional.ofNullable(scalarFunctionConverter); return this; } /** - * Sets the execution behavior for plans created by the resulting converter. + * Returns the explicitly configured scalar function converter, or {@link Optional#empty()} when + * it is to be derived from the configured extensions and type factory. * - * @param executionBehavior the execution behavior + * @return the configured scalar function converter, if any + */ + public Optional getScalarFunctionConverter() { + return scalarFunctionConverter; + } + + /** + * Sets the aggregate function converter. When left unset, it is derived from the configured + * extensions and type factory. + * + * @param aggregateFunctionConverter the aggregate function converter * @return this builder */ - public Builder executionBehavior(Plan.ExecutionBehavior executionBehavior) { - this.executionBehavior = executionBehavior; + public Builder aggregateFunctionConverter( + AggregateFunctionConverter aggregateFunctionConverter) { + this.aggregateFunctionConverter = Optional.ofNullable(aggregateFunctionConverter); return this; } /** - * Sets the full Calcite {@link SqlParser.Config} used for SQL parsing, replacing the default. + * Returns the explicitly configured aggregate function converter, or {@link Optional#empty()} + * when it is to be derived from the configured extensions and type factory. * - *

Use {@link ConverterProvider#DEFAULT_SQL_PARSER_CONFIG} as a starting point to retain - * isthmus' DDL parser factory and conformance while overriding individual settings. + * @return the configured aggregate function converter, if any + */ + public Optional getAggregateFunctionConverter() { + return aggregateFunctionConverter; + } + + /** + * Sets the window function converter. When left unset, it is derived from the configured + * extensions and type factory. * - * @param sqlParserConfig the parser configuration + * @param windowFunctionConverter the window function converter * @return this builder */ - public Builder sqlParserConfig(SqlParser.Config sqlParserConfig) { - this.sqlParserConfig = sqlParserConfig; + public Builder windowFunctionConverter(WindowFunctionConverter windowFunctionConverter) { + this.windowFunctionConverter = Optional.ofNullable(windowFunctionConverter); return this; } + /** + * Returns the explicitly configured window function converter, or {@link Optional#empty()} when + * it is to be derived from the configured extensions and type factory. + * + * @return the configured window function converter, if any + */ + public Optional getWindowFunctionConverter() { + return windowFunctionConverter; + } + /** * Builds a {@link ConverterProvider} from the configured components, deriving any unset * function converters from the configured extensions and type factory. diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index abc0288f3..382e1d58d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -87,16 +87,37 @@ public DynamicConverterProvider( /** * Creates a new DynamicConverterProvider from a {@link ConverterProvider.Builder}, seeding base * state from the builder and then installing the converter that handles dynamic extension - * functions. This is the seam through which subclasses route via {@code super(builder)}. + * functions. * * @param builder the builder carrying the configured components + * @throws IllegalArgumentException if the builder configures a scalar function converter, which + * this provider derives itself */ public DynamicConverterProvider(ConverterProvider.Builder builder) { - super(builder); + super(requireDerivedScalarFunctionConverter(builder)); this.scalarFunctionConverter = createScalarFunctionConverter(); this.operatorTable = buildSqlOperatorTable(); } + /** + * Rejects a builder that configures a scalar function converter: this provider derives its own + * from the dynamic extensions, so a configured one would be silently discarded. + * + * @param builder the builder to check + * @return the given builder + * @throws IllegalArgumentException if the builder configures a scalar function converter + */ + private static ConverterProvider.Builder requireDerivedScalarFunctionConverter( + ConverterProvider.Builder builder) { + if (builder.getScalarFunctionConverter().isPresent()) { + throw new IllegalArgumentException( + "DynamicConverterProvider derives its own scalar function converter from the dynamic " + + "extensions; remove the ConverterProvider.Builder.scalarFunctionConverter(...) " + + "setting"); + } + return builder; + } + /** * Returns the list of call converters, including dynamically generated converters for extension * functions. diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java index 1bb73763a..4e5679d92 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlStatementParser.java @@ -28,8 +28,7 @@ public static List parseStatements(String sqlStatements) throws SqlPars * the given {@link ConverterProvider}. * *

To use a custom parser configuration, build the {@link ConverterProvider} via {@link - * ConverterProvider#builder()} and its {@code sqlParserConfig(...)}; for fully dynamic behaviour, - * subclass {@link ConverterProvider} and override {@link ConverterProvider#getSqlParserConfig()}. + * ConverterProvider#builder()} and its {@code sqlParserConfig(...)}. * * @param sqlStatements a string containing one or more SQL statements * @param converterProvider the converter provider whose parser config controls identifier casing diff --git a/isthmus/src/test/java/io/substrait/isthmus/ConverterProviderBuilderTest.java b/isthmus/src/test/java/io/substrait/isthmus/ConverterProviderBuilderTest.java new file mode 100644 index 000000000..c3d0f7ba2 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/ConverterProviderBuilderTest.java @@ -0,0 +1,116 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.expression.AggregateFunctionConverter; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import io.substrait.isthmus.expression.WindowFunctionConverter; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.junit.jupiter.api.Test; + +class ConverterProviderBuilderTest { + + static final SimpleExtension.ExtensionCollection EXTENSIONS = + DefaultExtensionCatalog.DEFAULT_COLLECTION; + static final RelDataTypeFactory TYPE_FACTORY = SubstraitTypeSystem.TYPE_FACTORY; + + @Test + void derivesFunctionConvertersWhenUnset() { + ConverterProvider provider = ConverterProvider.builder().build(); + + assertNotNull(provider.getScalarFunctionConverter()); + assertNotNull(provider.getAggregateFunctionConverter()); + assertNotNull(provider.getWindowFunctionConverter()); + assertEquals(ConverterProvider.DEFAULT_SQL_PARSER_CONFIG, provider.getSqlParserConfig()); + } + + @Test + void usesExplicitlyConfiguredFunctionConverters() { + ScalarFunctionConverter sfc = scalarFunctionConverter(); + AggregateFunctionConverter afc = aggregateFunctionConverter(); + WindowFunctionConverter wfc = windowFunctionConverter(); + + ConverterProvider provider = + ConverterProvider.builder() + .scalarFunctionConverter(sfc) + .aggregateFunctionConverter(afc) + .windowFunctionConverter(wfc) + .build(); + + assertSame(sfc, provider.getScalarFunctionConverter()); + assertSame(afc, provider.getAggregateFunctionConverter()); + assertSame(wfc, provider.getWindowFunctionConverter()); + } + + @Test + void dynamicConverterProviderRejectsConfiguredScalarFunctionConverter() { + ConverterProvider.Builder builder = + ConverterProvider.builder().scalarFunctionConverter(scalarFunctionConverter()); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> new DynamicConverterProvider(builder)); + assertTrue(e.getMessage().contains("scalarFunctionConverter"), e.getMessage()); + } + + @Test + void dynamicConverterProviderAcceptsBuilderWithoutFunctionConverters() { + assertDoesNotThrow( + () -> new DynamicConverterProvider(ConverterProvider.builder().extensions(EXTENSIONS))); + } + + @Test + void automaticDynamicProviderRejectsConfiguredScalarFunctionConverter() { + assertRejected( + ConverterProvider.builder().scalarFunctionConverter(scalarFunctionConverter()), + "scalarFunctionConverter"); + } + + @Test + void automaticDynamicProviderRejectsConfiguredAggregateFunctionConverter() { + assertRejected( + ConverterProvider.builder().aggregateFunctionConverter(aggregateFunctionConverter()), + "aggregateFunctionConverter"); + } + + @Test + void automaticDynamicProviderRejectsConfiguredWindowFunctionConverter() { + assertRejected( + ConverterProvider.builder().windowFunctionConverter(windowFunctionConverter()), + "windowFunctionConverter"); + } + + @Test + void automaticDynamicProviderAcceptsBuilderWithoutFunctionConverters() { + assertDoesNotThrow( + () -> + new AutomaticDynamicFunctionMappingConverterProvider( + ConverterProvider.builder().extensions(EXTENSIONS))); + } + + private static void assertRejected(ConverterProvider.Builder builder, String setterName) { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> new AutomaticDynamicFunctionMappingConverterProvider(builder)); + assertTrue(e.getMessage().contains(setterName), e.getMessage()); + } + + private static ScalarFunctionConverter scalarFunctionConverter() { + return new ScalarFunctionConverter(EXTENSIONS.scalarFunctions(), TYPE_FACTORY); + } + + private static AggregateFunctionConverter aggregateFunctionConverter() { + return new AggregateFunctionConverter(EXTENSIONS.aggregateFunctions(), TYPE_FACTORY); + } + + private static WindowFunctionConverter windowFunctionConverter() { + return new WindowFunctionConverter(EXTENSIONS.windowFunctions(), TYPE_FACTORY); + } +}