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. |
vbarua
force-pushed
the
vbarua/conversion-configuration-refactor
branch
from
94eab31 to
685de52
Compare
vbarua
changed the title
vbarua
changed the title
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
At first glance, this looks like quite a reasonable approach to rationalizing different configuration options that are needed throughout the codebase. Just some very minor observations and comments in-line.
I notice that this PR is holding out #647. Do you think this is close to being ready to merge — at least as an initial step — so that can be unblocked?
isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java
Outdated
Show resolved
Hide resolved
vbarua
force-pushed
the
vbarua/conversion-configuration-refactor
branch
from
8a9e6bb to
7f4c3e4
Compare
Inject ConverterProvider into conversion classes to control behavior
vbarua
force-pushed
the
vbarua/conversion-configuration-refactor
branch
from
7f4c3e4 to
f1e4107
Compare
vbarua
force-pushed
the
vbarua/conversion-configuration-refactor
branch
from
0fe7d5d to
19833b6
Compare
| import org.apache.calcite.sql.SqlOperatorTable; | ||
| import org.apache.calcite.sql.util.SqlOperatorTables; | ||
|
|
||
| public class DynamicConverterProvider extends ConverterProvider { |
There was a problem hiding this comment.
This class represents the ultimate goal of this PR. Previously, all of the code for handling dynamic conversions was smeared over various parts of the codebase. Now, it's all in one place, which make it easier to reason about how the dynamic conversion behaviour is applied.
| public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { | ||
| super(features, extensions); | ||
|
|
||
| if (featureBoard.allowDynamicUdfs()) { |
There was a problem hiding this comment.
The behaviour below is now encapsulated in the DynamicConverterProvider
|
|
||
| List<FunctionMappings.Sig> additionalSignatures; | ||
|
|
||
| if (allowDynamicUdfs) { |
There was a problem hiding this comment.
The dynamic function handling code is now encapsulated in the DynamicConverterProvider
| ArrayList<CallConverter> converters = new ArrayList<>(); | ||
| converters.addAll(CallConverters.defaults(typeConverter)); | ||
|
|
||
| if (features.allowDynamicUdfs()) { |
There was a problem hiding this comment.
This code is now encapuslated in the DynamicConverterProvider
| FunctionMappings.s(customScalarListStringAndAnyFn), | ||
| FunctionMappings.s(customScalarListStringAndAnyVariadic0Fn), | ||
| FunctionMappings.s(customScalarListStringAndAnyVariadic1Fn), | ||
| FunctionMappings.s(toBType)); |
There was a problem hiding this comment.
Moved below so that it can be declared static
2 participants
Introduces the ConverterProvider class as a single source of configuration for
all conversion related classes between SQL, Calcite and Substrait. All
conversion classes now have constructors consuming a ConverterProvider.
The no-arg ConverterProvider constructor provides reasonable defaults for
conversions. Other constructors can override some behaviours, and full
customization can be achieved by extending the ConverterProvider, as is done in
DynamicConverterProvider
BREAKING CHANGE: removed FeatureBoard class
BREAKING CHANGE: all constructors consume a FeatureBoard have been removed
BREAKING CHANGE: a number of methods/constructors have been replaced with ConverterProvider equivalents