Context
Review discussion on #1036: now that ConverterProvider has a builder, there are two
customization mechanisms — the builder and subclassing — and they can be mixed. As
@vbarua noted,
it isn't obvious which one a user should reach for, and the built-in subclasses are
themselves an example of the ambiguity:
// today
new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions))
// could plausibly be
ConverterProvider.builder()
.extensions(extensions)
.scalarFunctionConverter(...)
.sqlOperatorTable(...)
.build()
What's missing
DynamicConverterProvider and AutomaticDynamicFunctionMappingConverterProvider subclass
rather than configure because the builder can't express what they override:
getSqlOperatorTable() — both chain dynamically generated operators onto the base table.
getCallConverters() — DynamicConverterProvider appends an extra ScalarFunctionConverter.
Both need the derived extensions/type factory to compute their values, so exposing them as
plain builder settings also means giving callers a way to build those pieces (e.g. a public
helper that turns an extension collection into the dynamic operators and signatures).
Proposal
- Add
sqlOperatorTable and callConverters to ConverterProvider.Builder.
- Expose the derivation helpers the dynamic providers use so a caller can assemble the same
configuration.
- Re-express both dynamic providers as builder configuration (static factory methods returning
a preconfigured Builder), and deprecate the subclasses.
- Document one recommended path: the builder for composition, subclassing only for behaviour
that must be computed per call.
Notes
#1036 added guards that throw IllegalArgumentException when a builder passed to either dynamic
provider configures a function converter those providers derive themselves — a stopgap that makes
the discarded-setting case loud. This issue is about removing the need for that overlap.
Context
Review discussion on #1036: now that
ConverterProviderhas a builder, there are twocustomization mechanisms — the builder and subclassing — and they can be mixed. As
@vbarua noted,
it isn't obvious which one a user should reach for, and the built-in subclasses are
themselves an example of the ambiguity:
What's missing
DynamicConverterProviderandAutomaticDynamicFunctionMappingConverterProvidersubclassrather than configure because the builder can't express what they override:
getSqlOperatorTable()— both chain dynamically generated operators onto the base table.getCallConverters()—DynamicConverterProviderappends an extraScalarFunctionConverter.Both need the derived extensions/type factory to compute their values, so exposing them as
plain builder settings also means giving callers a way to build those pieces (e.g. a public
helper that turns an extension collection into the dynamic operators and signatures).
Proposal
sqlOperatorTableandcallConverterstoConverterProvider.Builder.configuration.
a preconfigured
Builder), and deprecate the subclasses.that must be computed per call.
Notes
#1036 added guards that throw
IllegalArgumentExceptionwhen a builder passed to either dynamicprovider configures a function converter those providers derive themselves — a stopgap that makes
the discarded-setting case loud. This issue is about removing the need for that overlap.