feat(isthmus): introduce a ConverterProvider builder - #1036
Conversation
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.
…nverterProvider 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.
b065c1c to
defa554
Compare
…rter 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.
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Looks reasonable overall. I did leave a number of suggestions, but nothing I would consider blocking.
One bigger item I did want to point out is that now we have two ways of customizing the ConverterProvider using the builder or subclassing, and they can also be mixed, which does make this a bit confusing potentially.
| * @param builder the builder carrying the configured components | ||
| */ | ||
| public DynamicConverterProvider(ConverterProvider.Builder builder) { | ||
| super(builder); |
There was a problem hiding this comment.
Potentially out of scope. With both a builder and subclassing available as extension mechanisms, we do start to open ourselves up a bit to questions about which to use. For example
why use subclassing here when we could inject a modified ConverterProvider like:
ConverterProvider
.builder()
.scalarFunctionConverter(createScalarFunctionConverter())
.sqlOperatorTable(buildSqlOperatorTable())There was a problem hiding this comment.
Fair concern, and I agree it is the real design question here. I have left it out of this PR and opened #1048 for it.
The reason the two dynamic providers subclass rather than configure is that the builder cannot currently express what they override: getSqlOperatorTable() (both chain generated operators onto the base table) and getCallConverters() (DynamicConverterProvider appends an extra ScalarFunctionConverter). Making them builder configurations means adding sqlOperatorTable and callConverters to Builder and exposing the derivation helpers, since those values depend on the resolved extensions and type factory. That is a bigger change than this PR, and #1037 is stacked on top.
In the meantime the overlap is at least no longer silent: passing a builder-configured function converter to either dynamic provider now throws instead of being discarded.
| * ConverterProvider#getSqlParserConfig()}. | ||
| * <p>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()}. |
There was a problem hiding this comment.
minor: I think we can just point users to using the builder for customization for this use case, and not full on extension.
There was a problem hiding this comment.
Trimmed to point at the builder only.
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.
|
Thanks for the review! Pushed ff3176d addressing the suggestions:
On the bigger point about two customization mechanisms — I agree, and opened #1048 rather than growing this PR (the builder would need |
2 participants
Summary
Add
ConverterProvider.builder()and a nestedBuilder— a readable, forward-compatible way toconfigure a
ConverterProvider, most importantly to supply a full CalciteSqlParser.Confignowthat the parsers consume the provider's config (#1035).
ConverterProvider.DEFAULT_SQL_PARSER_CONFIG(
SqlParser.Config.DEFAULT+TO_UPPER+SqlDdlParserImpl.FACTORY+LENIENT) is exposed as thebase for customized configs, e.g.
DEFAULT_SQL_PARSER_CONFIG.withUnquotedCasing(Casing.UNCHANGED).Sole construction path + subclassing seam
All public
ConverterProviderconstructors are@Deprecatedin favour ofbuilder(), andDEFAULTis
builder().build(). Subclassing routes through a newprotected ConverterProvider(Builder)seam— the single place instances are created — so adding a component later touches only the builder and
this constructor, never a subclass
super(...)call.DynamicConverterProviderandAutomaticDynamicFunctionMappingConverterProviderconstruct viasuper(builder).Now that the provider is fully configurable, call sites that still used global defaults are routed
through it: the builder's
typeConverterreaches the scalar/aggregate/window converters, and theCREATE-statement catalog reader uses the provider's type factory and connection config.
Compatibility
Source-compatible — no public API removed, deprecated constructors still delegate to the builder,
and behavior is unchanged for the default provider.
Stack (#983 split, merge bottom-up)
ConverterProviderinto the SQL statement parsers (merged)