-
Notifications
You must be signed in to change notification settings - Fork 93
feat: introduce ConverterProvider to control conversion behaviour #649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: