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..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; @@ -47,9 +48,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 +64,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 +83,28 @@ 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. + * + * @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(requireDerivedFunctionConverters(builder)); List dynamicScalarOperators = getDynamicScalarOperators(); this.scalarFunctionConverter = createScalarFunctionConverter(dynamicScalarOperators); @@ -93,6 +122,32 @@ public AutomaticDynamicFunctionMappingConverterProvider( 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 f8b4b130e..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; @@ -40,21 +41,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 +102,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 +130,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 +148,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 +159,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 +179,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 +191,54 @@ 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)); + } + + /** + * 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.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)); } /** @@ -178,13 +258,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 +505,182 @@ 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 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. + * + * @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 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; + } + + /** + * 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 = Optional.ofNullable(scalarFunctionConverter); + return this; + } + + /** + * Returns the explicitly configured scalar function converter, or {@link Optional#empty()} when + * it is to be derived from the configured extensions and type factory. + * + * @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 aggregateFunctionConverter( + AggregateFunctionConverter aggregateFunctionConverter) { + this.aggregateFunctionConverter = Optional.ofNullable(aggregateFunctionConverter); + return this; + } + + /** + * Returns the explicitly configured aggregate function converter, or {@link Optional#empty()} + * when it is to be derived from the configured extensions and type factory. + * + * @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 windowFunctionConverter the window function converter + * @return this builder + */ + 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. + * + * @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..382e1d58d 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,14 +74,50 @@ 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. + * + * @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(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/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()); } 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/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()); } /** 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..4e5679d92 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,8 @@ 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(...)}. * * @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/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); + } +} 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);