feat: introduce ConverterProvider to control conversion behaviour#649
feat: introduce ConverterProvider to control conversion behaviour#649
Conversation
There was a problem hiding this comment.
I'm experimenting with overhauling the converter configuration to make changes like #647 easier to make, and to handle some of the concerns I had with #457 (review).
I still need to figure out how to structure the code to make it easy to utilize the DynamicConverterProvider, but I wanted to share the kernel of the idea.
| import org.apache.calcite.rel.type.RelDataTypeFactory; | ||
| import org.apache.calcite.rex.RexBuilder; | ||
|
|
||
| public class ConverterProvider { |
There was a problem hiding this comment.
We wire in the same 4 converters (3 function converter + 1 type converter) in a number of places (SubstraitRelVisitor, SubstraitRelNodeConverter, ...). Instead of doing that, we can centralized the configuration of converters into one provider*. This would also allow us to provide extended providers, like the DynamicConverterProvider, that could implement the dynamic fallback behaviour in a single location rather that sprinkling it throughout the various parts of the codebase, which makes it very difficult to reason about.
* I'm not set on the name
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
Inject ConverterProvider into conversion classes to control behavior
94eab31 to
685de52
Compare
ConverterProvider is a new class consumed by the various classes used to convert
between SQL <-> Calcite <-> Substrait
Its usage allows us to centralize the configuration of conversions behaviours,
which both helps to: