You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
// todaynewDynamicConverterProvider(ConverterProvider.builder().extensions(extensions))
// could plausibly beConverterProvider.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.
getSqlOperatorTable() needs the derived extensions/type factory to compute its value, so
exposing it as a plain builder setting also means giving callers a way to build those pieces
(e.g. a public helper that turns an extension collection into the dynamic operators).
Proposal
Add callConverters to ConverterProvider.Builder.Done in feat(isthmus): let the builder transform the call converter list #1166, as callConverters(UnaryOperator<List<CallConverter>>) — a transform over the assembled list
rather than a replacement for it. A plain list cannot work here: scalarFunctionConverter goes
last in getCallConverters(), is derived during construction, and DynamicConverterProvider
reassigns it after super(builder) returns, so anything frozen at build() captures the
pre-override converter; a caller supplying the whole list themselves would also trip the feat(isthmus): introduce a ConverterProvider builder #1036
guards. The transform is handed the built-ins with late-bound values in place and runs per
conversion, so it expresses insert-at-position, append, prepend, replace and filter.
Add sqlOperatorTable to ConverterProvider.Builder. Whether this wants the same transform
shape is open — unlike the converter list it has no derived-late component, so a plain setting
may be enough.
Expose the derivation helpers the dynamic providers use so a caller can assemble the same
configuration. Smaller than originally scoped: no helper is needed for the call-converter half,
since the transform already receives the derived converter.
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.Addressed by feat(isthmus): let the builder transform the call converter list #1166; see the proposal below. That append turns out to be a duplicate that never
fires (refactor(isthmus): DynamicConverterProvider appends a duplicate ScalarFunctionConverter that never fires #1208), so this half may be a deletion rather than a migration.
getSqlOperatorTable()needs the derived extensions/type factory to compute its value, soexposing it as a plain builder setting also means giving callers a way to build those pieces
(e.g. a public helper that turns an extension collection into the dynamic operators).
Proposal
AddDone in feat(isthmus): let the builder transform the call converter list #1166, ascallConverterstoConverterProvider.Builder.callConverters(UnaryOperator<List<CallConverter>>)— a transform over the assembled listrather than a replacement for it. A plain list cannot work here:
scalarFunctionConvertergoeslast in
getCallConverters(), is derived during construction, andDynamicConverterProviderreassigns it after
super(builder)returns, so anything frozen atbuild()captures thepre-override converter; a caller supplying the whole list themselves would also trip the feat(isthmus): introduce a ConverterProvider builder #1036
guards. The transform is handed the built-ins with late-bound values in place and runs per
conversion, so it expresses insert-at-position, append, prepend, replace and filter.
sqlOperatorTabletoConverterProvider.Builder. Whether this wants the same transformshape is open — unlike the converter list it has no derived-late component, so a plain setting
may be enough.
configuration. Smaller than originally scoped: no helper is needed for the call-converter half,
since the transform already receives the derived converter.
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.