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
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?
| /** Use {@link SqlToSubstrait#SqlToSubstrait(ConverterProvider)} instead */ | ||
| @Deprecated | ||
| public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions) { | ||
| this(new ConverterProvider(extensions)); |
There was a problem hiding this comment.
It seems strange to define a new constructor that is already deprecated. I wonder if it should not exist at all.
| /** Use {@link SubstraitRelVisitor#SubstraitRelVisitor(ConverterProvider)} */ | ||
| @Deprecated | ||
| public SubstraitRelVisitor( | ||
| RelDataTypeFactory typeFactory, | ||
| ScalarFunctionConverter scalarFunctionConverter, | ||
| AggregateFunctionConverter aggregateFunctionConverter, | ||
| WindowFunctionConverter windowFunctionConverter, | ||
| TypeConverter typeConverter, | ||
| FeatureBoard features) { | ||
| ArrayList<CallConverter> converters = new ArrayList<CallConverter>(); | ||
| converters.addAll(CallConverters.defaults(typeConverter)); | ||
| converters.add(scalarFunctionConverter); | ||
| converters.add(CallConverters.CREATE_SEARCH_CONV.apply(new RexBuilder(typeFactory))); | ||
| this.aggregateFunctionConverter = aggregateFunctionConverter; | ||
| this.rexExpressionConverter = | ||
| new RexExpressionConverter(this, converters, windowFunctionConverter, typeConverter); | ||
| this.typeConverter = typeConverter; | ||
| this.featureBoard = features; | ||
| TypeConverter typeConverter) { |
There was a problem hiding this comment.
The constructor signature has changed anyway so this is effectively a new constructor that replaces one we no longer want people to use. Since any calling code needs to be changed, perhaps it would be better to to introduce a new (deprecated) constructor and just have them make the desired change first time.
| SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedDynamicOperators)); | ||
| } | ||
|
|
||
| return SubstraitOperatorTable.INSTANCE; |
There was a problem hiding this comment.
Maybe this should get the default value from the superclass method.
| if (!dynamicExtensionCollection.scalarFunctions().isEmpty() | ||
| || !dynamicExtensionCollection.aggregateFunctions().isEmpty()) { | ||
| List<SqlOperator> generatedDynamicOperators = | ||
| SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); | ||
| return SqlOperatorTables.chain( | ||
| SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedDynamicOperators)); | ||
| } |
There was a problem hiding this comment.
Very minor style suggestion: testing if both are empty gives the very simple short-circuit case where the default value is returned, which means less indented code and might improve readability.
2 participants
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: