feat(isthmus): let the builder transform the call converter list - #1166
Conversation
da5bf43 to
a390ef8
Compare
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Replace additionalCallConverters(List) with callConverters(UnaryOperator<List<CallConverter>>) applied inside getCallConverters(), so a caller orders converters explicitly instead of merging against a list they can't name. Prepending is what I'd hold this on — it puts caller converters ahead of CAST, REINTERPRET and ROW, which are Isthmus' encoding of nullable and user-defined literals rather than dialect behaviour. A transform also makes append expressible, which is what #1048 item 3 needs to retire the dynamic subclasses.
68dfbf4 to
49bc526
Compare
alexandrefimov
changed the title
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Transform shape looks right, and the window test earns its place. One contract left implicit.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Add one sentence saying a converter must not hand the call it is claiming back to topLevelConverter. Prepending to adjust a built-in's result is exactly the idiom the paragraph above invites, and it recurses without end — topLevelConverter is rexNode -> rexNode.accept(this), which re-enters the list from the start. Otherwise this is ready: the transform shape and the mutability test both read right.
ConverterProvider.getCallConverters() assembles its list internally, so a consumer whose Calcite dialect emits calls the built-in converters do not handle has to subclass the provider to add one. The builder already takes the scalar, aggregate and window function converters; call converters are the remaining piece. Additional converters are consulted ahead of the built-in ones, so a dialect that gives a call different semantics can claim it — a subclass overriding getCallConverters() could already place its converters anywhere in the list, and a hook that can only append would be weaker than the subclassing it is meant to replace. Part of substrait-io#1048.
The tests assembled a RexExpressionConverter out of the provider's parts, so they pinned getCallConverters() and not whether the added converters reach a conversion: routing getRexExpressionConverter around it left them green.
A list to prepend decides the order for the caller, and prepending is the wrong default: it puts a caller's converters ahead of CAST, REINTERPRET and ROW, which are this repository's encoding of nullable and user-defined literals rather than dialect behaviour. A transform lets the caller say where its own converters sit, and makes appending expressible as well. It also has to run where the list is assembled rather than at build time: the scalar function converter is derived during construction and DynamicConverterProvider reassigns it after super(builder) returns, so anything frozen at build() captures the wrong one. The javadoc said a windowed call does not reach these converters. Its operands, partition keys and sort keys do -- only the RexOver node itself does not, along with an aggregate call, a subquery, and the reference expression of a field access outside the kinds it walks through.
DynamicConverterProvider appends to what super.getCallConverters() returns, so a transform ending in an unmodifiable list throws at conversion time, for that provider alone. Said in the javadoc and pinned by a test.
1449cc7 to
f7d0a93
Compare
2 participants
ConverterProvider.getCallConverters()assembles its list internally, so a consumer whose Calcite dialect emits calls the built-in converters do not handle has to subclass the provider to add one. The builder already takes the scalar, aggregate and window function converters; call converters are the remaining piece. This is item 1 of #1048 —sqlOperatorTableis untouched here.The builder takes a transform over the assembled list rather than converters to prepend. Where a caller's converter sits is a decision only the caller can make: ahead of a built-in one it claims a call that one would otherwise take, which is what a dialect giving a call different semantics needs, and also what puts it ahead of
CAST,REINTERPRETandROW— this repository's encoding of nullable and user-defined literals rather than dialect behaviour. Behind them it catches only what none of them claims. A transform says both, and it says replace and remove as well, which is what retiring the dynamic subclasses in item 3 of that issue needs.The transform is applied where the list is assembled rather than at build time, so it sees the type converter and the scalar function converter as they are then. That matters here: the scalar function converter is derived during construction, and
DynamicConverterProviderreassigns it aftersuper(builder)returns, so anything frozen atbuild()would capture the wrong one. Both dynamic providers keep working —AutomaticDynamicFunctionMappingConverterProviderdoes not overridegetCallConverters(), andDynamicConverterProviderappends to whatsuperreturns, which is now the caller's list.The case this comes from is an external consumer exporting Impala's Calcite plans through Isthmus: Impala rewrites
IFandcastinto operators carryingSqlKind.OTHER, whichCallConverters.CASEandCallConverters.CASTdecline, so today it subclassesConverterProviderfor both.What does not reach these converters is narrower than an earlier revision of this description claimed: the
RexOvernode of a windowed call, an aggregate call, a subquery, and the reference expression of a field access outside the item, input-reference and field-access kinds it walks through. A windowed call's operands, partition keys and sort keys are converted through the list as usual, whichareConsultedForTheExpressionsInsideAWindowedCallpins. The extended-expression path is honoured too now that #1168 takes its converter from the provider.Part of #1048.