From 456776462e88096e6605a381990676b073adecb5 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 18 Dec 2025 21:19:47 -0800 Subject: [PATCH 01/28] feat: make SqlMapValueConstructorCallConverter public --- .../isthmus/expression/SqlMapValueConstructorCallConverter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/SqlMapValueConstructorCallConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/SqlMapValueConstructorCallConverter.java index 8cf4958d8..65d24a0ed 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/SqlMapValueConstructorCallConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/SqlMapValueConstructorCallConverter.java @@ -15,7 +15,7 @@ public class SqlMapValueConstructorCallConverter implements CallConverter { - SqlMapValueConstructorCallConverter() {} + public SqlMapValueConstructorCallConverter() {} @Override public Optional convert( From ce3bb774fe16087e473657df4b036330a5990db7 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 18 Dec 2025 21:43:20 -0800 Subject: [PATCH 02/28] feat: introduce ConverterProvider to centralize converter configuration --- .../substrait/isthmus/ConverterProvider.java | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java new file mode 100644 index 000000000..6aad6244a --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -0,0 +1,77 @@ +package io.substrait.isthmus; + +import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.expression.AggregateFunctionConverter; +import io.substrait.isthmus.expression.CallConverters; +import io.substrait.isthmus.expression.FieldSelectionConverter; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import io.substrait.isthmus.expression.SqlArrayValueConstructorCallConverter; +import io.substrait.isthmus.expression.SqlMapValueConstructorCallConverter; +import io.substrait.isthmus.expression.WindowFunctionConverter; +import java.util.ArrayList; +import java.util.List; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; + +public class ConverterProvider { + + protected final RelDataTypeFactory typeFactory; + + private final ScalarFunctionConverter scalarFunctionConverter; + private final AggregateFunctionConverter aggregateFunctionConverter; + private final WindowFunctionConverter windowFunctionConverter; + + private final TypeConverter typeConverter; + + public ConverterProvider( + RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { + this( + typeFactory, + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), + new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory), + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), + TypeConverter.DEFAULT); + } + + public ConverterProvider( + RelDataTypeFactory typeFactory, + ScalarFunctionConverter sfc, + AggregateFunctionConverter afc, + WindowFunctionConverter wfc, + TypeConverter tc) { + this.typeFactory = typeFactory; + this.scalarFunctionConverter = sfc; + this.aggregateFunctionConverter = afc; + this.windowFunctionConverter = wfc; + this.typeConverter = tc; + } + + protected List getCallConverters() { + ArrayList callConverters = new ArrayList<>(); + callConverters.add(new FieldSelectionConverter(typeConverter)); + callConverters.add(CallConverters.CASE); + callConverters.add(CallConverters.CAST.apply(typeConverter)); + callConverters.add(CallConverters.REINTERPRET.apply(typeConverter)); + callConverters.add(new SqlArrayValueConstructorCallConverter(typeConverter)); + callConverters.add(new SqlMapValueConstructorCallConverter()); + callConverters.add(CallConverters.CREATE_SEARCH_CONV.apply(new RexBuilder(typeFactory))); + callConverters.add(scalarFunctionConverter); + return callConverters; + } + + public ScalarFunctionConverter getScalarFunctionConverter() { + return scalarFunctionConverter; + } + + public AggregateFunctionConverter getAggregateFunctionConverter() { + return aggregateFunctionConverter; + } + + public WindowFunctionConverter getWindowFunctionConverter() { + return windowFunctionConverter; + } + + public TypeConverter getTypeConverter() { + return typeConverter; + } +} From 25da2a97c5ec68c4151184722f8828a677f6f894 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 18 Dec 2025 21:44:10 -0800 Subject: [PATCH 03/28] refactor: utilize ConverterProvider in SubstraitRelVisitor --- .../isthmus/SubstraitRelVisitor.java | 64 ++++++------------- 1 file changed, 18 insertions(+), 46 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 26cf37c0c..3253abd2a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -8,8 +8,6 @@ import io.substrait.isthmus.calcite.rel.CreateTable; import io.substrait.isthmus.calcite.rel.CreateView; import io.substrait.isthmus.expression.AggregateFunctionConverter; -import io.substrait.isthmus.expression.CallConverters; -import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.LiteralConverter; import io.substrait.isthmus.expression.RexExpressionConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; @@ -58,11 +56,9 @@ import org.apache.calcite.rel.core.TableModify; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; -import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexFieldAccess; import org.apache.calcite.rex.RexInputRef; import org.apache.calcite.rex.RexNode; -import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.util.ImmutableBitSet; import org.immutables.value.Value; @@ -77,7 +73,6 @@ public class SubstraitRelVisitor extends RelNodeVisitor { protected final RexExpressionConverter rexExpressionConverter; protected final AggregateFunctionConverter aggregateFunctionConverter; protected final TypeConverter typeConverter; - protected final FeatureBoard featureBoard; private Map fieldAccessDepthMap; public SubstraitRelVisitor( @@ -89,41 +84,11 @@ public SubstraitRelVisitor( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { - - this.typeConverter = TypeConverter.DEFAULT; - ArrayList converters = new ArrayList<>(); - converters.addAll(CallConverters.defaults(typeConverter)); - - if (features.allowDynamicUdfs()) { - SimpleExtension.ExtensionCollection dynamicExtensionCollection = - ExtensionUtils.getDynamicExtensions(extensions); - List dynamicOperators = - SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); - - List additionalSignatures = - dynamicOperators.stream() - .map(op -> FunctionMappings.s(op, op.getName())) - .collect(Collectors.toList()); - converters.add( - new ScalarFunctionConverter( - extensions.scalarFunctions(), - additionalSignatures, - typeFactory, - TypeConverter.DEFAULT)); - } else { - converters.add(new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory)); - } - - converters.add(CallConverters.CREATE_SEARCH_CONV.apply(new RexBuilder(typeFactory))); - this.aggregateFunctionConverter = - new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory); - WindowFunctionConverter windowFunctionConverter = - new WindowFunctionConverter(extensions.windowFunctions(), typeFactory); - this.rexExpressionConverter = - new RexExpressionConverter(this, converters, windowFunctionConverter, typeConverter); - this.featureBoard = features; + this(new ConverterProvider(typeFactory, extensions)); } + /** Use {@link SubstraitRelVisitor#SubstraitRelVisitor(ConverterProvider)} */ + @Deprecated public SubstraitRelVisitor( RelDataTypeFactory typeFactory, ScalarFunctionConverter scalarFunctionConverter, @@ -131,15 +96,22 @@ public SubstraitRelVisitor( WindowFunctionConverter windowFunctionConverter, TypeConverter typeConverter, FeatureBoard features) { - ArrayList converters = new ArrayList(); - converters.addAll(CallConverters.defaults(typeConverter)); - converters.add(scalarFunctionConverter); - converters.add(CallConverters.CREATE_SEARCH_CONV.apply(new RexBuilder(typeFactory))); - this.aggregateFunctionConverter = aggregateFunctionConverter; + this( + new ConverterProvider( + typeFactory, + scalarFunctionConverter, + aggregateFunctionConverter, + windowFunctionConverter, + typeConverter)); + } + + public SubstraitRelVisitor(ConverterProvider converterProvider) { + List converters = converterProvider.getCallConverters(); + this.typeConverter = converterProvider.getTypeConverter(); + this.aggregateFunctionConverter = converterProvider.getAggregateFunctionConverter(); this.rexExpressionConverter = - new RexExpressionConverter(this, converters, windowFunctionConverter, typeConverter); - this.typeConverter = typeConverter; - this.featureBoard = features; + new RexExpressionConverter( + this, converters, converterProvider.getWindowFunctionConverter(), typeConverter); } protected Expression toExpression(RexNode node) { From e543109f877f0f8b7d4b7a111cd16e8e024480cf Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 18 Dec 2025 22:10:08 -0800 Subject: [PATCH 04/28] refactor: utilize ConverterProvider in SubstraitRelNodeConverter --- .../isthmus/SubstraitRelNodeConverter.java | 78 +++++++------------ 1 file changed, 27 insertions(+), 51 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index e1e6b6e21..200788ed5 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -119,7 +119,7 @@ public SubstraitRelNodeConverter( this( typeFactory, relBuilder, - createScalarFunctionConverter(extensions, typeFactory, featureBoard.allowDynamicUdfs()), + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory), new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), TypeConverter.DEFAULT); @@ -133,14 +133,15 @@ public SubstraitRelNodeConverter( WindowFunctionConverter windowFunctionConverter, TypeConverter typeConverter) { this( - typeFactory, relBuilder, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter, new ExpressionRexConverter( - typeFactory, scalarFunctionConverter, windowFunctionConverter, typeConverter)); + typeFactory, scalarFunctionConverter, windowFunctionConverter, typeConverter), + new ConverterProvider( + typeFactory, + scalarFunctionConverter, + aggregateFunctionConverter, + windowFunctionConverter, + typeConverter)); } public SubstraitRelNodeConverter( @@ -151,56 +152,31 @@ public SubstraitRelNodeConverter( WindowFunctionConverter windowFunctionConverter, TypeConverter typeConverter, ExpressionRexConverter expressionRexConverter) { - this.typeFactory = typeFactory; - this.typeConverter = typeConverter; + this( + relBuilder, + expressionRexConverter, + new ConverterProvider( + typeFactory, + scalarFunctionConverter, + aggregateFunctionConverter, + windowFunctionConverter, + typeConverter)); + } + + public SubstraitRelNodeConverter( + RelBuilder relBuilder, + ExpressionRexConverter expressionRexConverter, + ConverterProvider converterProvider) { + this.typeFactory = converterProvider.typeFactory; + this.typeConverter = converterProvider.getTypeConverter(); this.relBuilder = relBuilder; this.rexBuilder = new RexBuilder(typeFactory); - this.scalarFunctionConverter = scalarFunctionConverter; - this.aggregateFunctionConverter = aggregateFunctionConverter; + this.scalarFunctionConverter = converterProvider.getScalarFunctionConverter(); + this.aggregateFunctionConverter = converterProvider.getAggregateFunctionConverter(); this.expressionRexConverter = expressionRexConverter; this.expressionRexConverter.setRelNodeConverter(this); } - private static ScalarFunctionConverter createScalarFunctionConverter( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - boolean allowDynamicUdfs) { - - List additionalSignatures; - - if (allowDynamicUdfs) { - java.util.Set knownFunctionNames = - FunctionMappings.SCALAR_SIGS.stream() - .map(FunctionMappings.Sig::name) - .collect(Collectors.toSet()); - - List dynamicFunctions = - extensions.scalarFunctions().stream() - .filter(f -> !knownFunctionNames.contains(f.name().toLowerCase())) - .collect(Collectors.toList()); - - if (dynamicFunctions.isEmpty()) { - additionalSignatures = Collections.emptyList(); - } else { - SimpleExtension.ExtensionCollection dynamicExtensionCollection = - SimpleExtension.ExtensionCollection.builder().scalarFunctions(dynamicFunctions).build(); - - List dynamicOperators = - SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); - - additionalSignatures = - dynamicOperators.stream() - .map(op -> FunctionMappings.s(op, op.getName())) - .collect(Collectors.toList()); - } - } else { - additionalSignatures = Collections.emptyList(); - } - - return new ScalarFunctionConverter( - extensions.scalarFunctions(), additionalSignatures, typeFactory, TypeConverter.DEFAULT); - } - public static RelNode convert( Rel relRoot, RelOptCluster relOptCluster, From cf488e6112a348955caf22f3eca78e59f9740961 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 18 Dec 2025 21:53:01 -0800 Subject: [PATCH 05/28] feat: introduce DynamicConverterProvider --- .../isthmus/DynamicConverterProvider.java | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java new file mode 100644 index 000000000..811292eba --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -0,0 +1,84 @@ +package io.substrait.isthmus; + +import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.expression.FunctionMappings; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.sql.SqlOperator; + +public class DynamicConverterProvider extends ConverterProvider { + + private final SimpleExtension.ExtensionCollection extensions; + + private final ScalarFunctionConverter scalarFunctionConverter; + + public DynamicConverterProvider( + RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { + super(typeFactory, extensions); + this.extensions = extensions; + this.scalarFunctionConverter = createScalarFunctionConverter(extensions, typeFactory); + } + + @Override + protected List getCallConverters() { + List callConverters = super.getCallConverters(); + + SimpleExtension.ExtensionCollection dynamicExtensionCollection = + ExtensionUtils.getDynamicExtensions(extensions); + List dynamicOperators = + SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); + List additionalSignatures = + dynamicOperators.stream() + .map(op -> FunctionMappings.s(op, op.getName())) + .collect(Collectors.toList()); + callConverters.add( + new ScalarFunctionConverter( + extensions.scalarFunctions(), + additionalSignatures, + typeFactory, + TypeConverter.DEFAULT)); + return callConverters; + } + + @Override + public ScalarFunctionConverter getScalarFunctionConverter() { + return scalarFunctionConverter; + } + + private static ScalarFunctionConverter createScalarFunctionConverter( + SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { + + List additionalSignatures; + + java.util.Set knownFunctionNames = + FunctionMappings.SCALAR_SIGS.stream() + .map(FunctionMappings.Sig::name) + .collect(Collectors.toSet()); + + List dynamicFunctions = + extensions.scalarFunctions().stream() + .filter(f -> !knownFunctionNames.contains(f.name().toLowerCase())) + .collect(Collectors.toList()); + + if (dynamicFunctions.isEmpty()) { + additionalSignatures = Collections.emptyList(); + } else { + SimpleExtension.ExtensionCollection dynamicExtensionCollection = + SimpleExtension.ExtensionCollection.builder().scalarFunctions(dynamicFunctions).build(); + + List dynamicOperators = + SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); + + additionalSignatures = + dynamicOperators.stream() + .map(op -> FunctionMappings.s(op, op.getName())) + .collect(Collectors.toList()); + } + + return new ScalarFunctionConverter( + extensions.scalarFunctions(), additionalSignatures, typeFactory, TypeConverter.DEFAULT); + } +} From b44fc37312c71687b8710b1d24b1a5dd129e3ef7 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Sun, 21 Dec 2025 20:24:34 -0800 Subject: [PATCH 06/28] feat: new SchemaCollector(ConverterProvider) constructor --- .../src/main/java/io/substrait/isthmus/SchemaCollector.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java index 99eaac1ab..45581b2fb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java @@ -30,6 +30,11 @@ public SchemaCollector(RelDataTypeFactory typeFactory, TypeConverter typeConvert this.typeConverter = typeConverter; } + public SchemaCollector(ConverterProvider converterProvider) { + this.typeFactory = converterProvider.getTypeFactory(); + this.typeConverter = converterProvider.getTypeConverter(); + } + /** * Returns a {@link CalciteSchema} containing all tables and schemas defined in {@link NamedScan}s * and {@link NamedWrite}s within the provided relation operation tree. From 70a10648d5536f73366c4b9ebf8aec8b11eb168e Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Sun, 21 Dec 2025 20:37:39 -0800 Subject: [PATCH 07/28] feat: configurable SubstraitRelNodeConverter and SubstraitToCalcite Inject ConverterProvider into conversion classes to control behavior --- .../substrait/isthmus/ConverterProvider.java | 20 ++++++++++ .../isthmus/SubstraitRelNodeConverter.java | 3 +- .../substrait/isthmus/SubstraitToCalcite.java | 33 +++++++-------- .../substrait/isthmus/CustomFunctionTest.java | 40 +++---------------- 4 files changed, 42 insertions(+), 54 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 6aad6244a..3b8926df2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -1,5 +1,6 @@ package io.substrait.isthmus; +import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.CallConverters; @@ -12,6 +13,7 @@ import java.util.List; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.tools.RelBuilder; public class ConverterProvider { @@ -23,6 +25,10 @@ public class ConverterProvider { private final TypeConverter typeConverter; + public ConverterProvider() { + this(SubstraitTypeSystem.TYPE_FACTORY, DefaultExtensionCatalog.DEFAULT_COLLECTION); + } + public ConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { this( @@ -59,6 +65,20 @@ protected List getCallConverters() { return callConverters; } + protected SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relBuilder) { + return new SubstraitRelNodeConverter( + typeFactory, + relBuilder, + getScalarFunctionConverter(), + getAggregateFunctionConverter(), + getWindowFunctionConverter(), + typeConverter); + } + + public RelDataTypeFactory getTypeFactory() { + return typeFactory; + } + public ScalarFunctionConverter getScalarFunctionConverter() { return scalarFunctionConverter; } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 200788ed5..66b221c8c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -12,7 +12,6 @@ import io.substrait.isthmus.calcite.rel.CreateView; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.ExpressionRexConverter; -import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.ScalarFunctionConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; import io.substrait.relation.AbstractDdlRel; @@ -167,7 +166,7 @@ public SubstraitRelNodeConverter( RelBuilder relBuilder, ExpressionRexConverter expressionRexConverter, ConverterProvider converterProvider) { - this.typeFactory = converterProvider.typeFactory; + this.typeFactory = converterProvider.getTypeFactory(); this.typeConverter = converterProvider.getTypeConverter(); this.relBuilder = relBuilder; this.rexBuilder = new RexBuilder(typeFactory); diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java index a0c5132e4..60a0e5e39 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java @@ -28,11 +28,9 @@ */ public class SubstraitToCalcite { - protected final SimpleExtension.ExtensionCollection extensions; protected final RelDataTypeFactory typeFactory; - protected final TypeConverter typeConverter; protected final Prepare.CatalogReader catalogReader; - protected final FeatureBoard featureBoard; + protected ConverterProvider converterProvider; public SubstraitToCalcite( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { @@ -72,11 +70,18 @@ public SubstraitToCalcite( TypeConverter typeConverter, Prepare.CatalogReader catalogReader, FeatureBoard featureBoard) { - this.extensions = extensions; - this.typeFactory = typeFactory; - this.typeConverter = typeConverter; + this(new ConverterProvider(typeFactory, extensions), catalogReader); + } + + public SubstraitToCalcite(ConverterProvider converterProvider) { + this(converterProvider, null); + } + + public SubstraitToCalcite( + ConverterProvider converterProvider, Prepare.CatalogReader catalogReader) { + this.converterProvider = converterProvider; + this.typeFactory = converterProvider.getTypeFactory(); this.catalogReader = catalogReader; - this.featureBoard = featureBoard; } /** @@ -85,7 +90,7 @@ public SubstraitToCalcite( *

Override this method to customize schema extraction. */ protected CalciteSchema toSchema(Rel rel) { - SchemaCollector schemaCollector = new SchemaCollector(typeFactory, typeConverter); + SchemaCollector schemaCollector = new SchemaCollector(converterProvider); return schemaCollector.toSchema(rel); } @@ -98,15 +103,6 @@ protected RelBuilder createRelBuilder(CalciteSchema schema) { return RelBuilder.create(Frameworks.newConfigBuilder().defaultSchema(schema.plus()).build()); } - /** - * Creates a {@link SubstraitRelNodeConverter} from the {@link RelBuilder} - * - *

Override this method to customize the {@link SubstraitRelNodeConverter}. - */ - protected SubstraitRelNodeConverter createSubstraitRelNodeConverter(RelBuilder relBuilder) { - return new SubstraitRelNodeConverter(extensions, typeFactory, relBuilder, featureBoard); - } - /** * Converts a Substrait {@link Rel} to a Calcite {@link RelNode} * @@ -125,7 +121,8 @@ public RelNode convert(Rel rel) { CalciteSchema rootSchema = toSchema(rel); relBuilder = createRelBuilder(rootSchema); } - SubstraitRelNodeConverter converter = createSubstraitRelNodeConverter(relBuilder); + SubstraitRelNodeConverter converter = + converterProvider.getSubstraitRelNodeConverter(relBuilder); return rel.accept(converter, Context.newContext()); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java index 9105316ac..79633775b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java @@ -22,7 +22,6 @@ import java.util.List; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.type.RelDataType; -import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeSystem; import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlFunction; @@ -31,7 +30,6 @@ import org.apache.calcite.sql.type.ReturnTypes; import org.apache.calcite.sql.type.SqlTypeFactoryImpl; import org.apache.calcite.sql.type.SqlTypeName; -import org.apache.calcite.tools.RelBuilder; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; @@ -246,44 +244,18 @@ public RelDataType toCalcite(Type.UserDefined type) { WindowFunctionConverter windowFunctionConverter = new WindowFunctionConverter(extensionCollection.windowFunctions(), typeFactory); - final SubstraitToCalcite substraitToCalcite = - new CustomSubstraitToCalcite(extensionCollection, typeFactory, typeConverter); - - // Create a SubstraitRelVisitor that uses the custom Function Converters - final SubstraitRelVisitor calciteToSubstrait = - new SubstraitRelVisitor( + ConverterProvider converterProvider = + new ConverterProvider( typeFactory, scalarFunctionConverter, aggregateFunctionConverter, windowFunctionConverter, - typeConverter, - ImmutableFeatureBoard.builder().build()); - - CustomFunctionTest() { - super(extensionCollection); - } - - // Create a SubstraitToCalcite converter that has access to the custom Function Converters - class CustomSubstraitToCalcite extends SubstraitToCalcite { + typeConverter); - public CustomSubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - TypeConverter typeConverter) { - super(extensions, typeFactory, typeConverter); - } + final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider); - @Override - protected SubstraitRelNodeConverter createSubstraitRelNodeConverter(RelBuilder relBuilder) { - return new SubstraitRelNodeConverter( - typeFactory, - relBuilder, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter); - } - } + // Create a SubstraitRelVisitor that uses the custom Function Converters + final SubstraitRelVisitor calciteToSubstrait = new SubstraitRelVisitor(converterProvider); @Test void customScalarFunctionRoundtrip() { From 588bb392c47aa6eb1945c738b40b0978cfea51b5 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 09:33:32 -0800 Subject: [PATCH 08/28] refactor: remove deprecated SqlToSubstrait#execute method --- .../io/substrait/isthmus/SqlToSubstrait.java | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index e60494244..b999fcf85 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -48,26 +48,6 @@ public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions, FeatureBoa this.operatorTable = SubstraitOperatorTable.INSTANCE; } - /** - * Converts one or more SQL statements into a Substrait {@link io.substrait.proto.Plan}. - * - * @param sqlStatements a string containing one more SQL statements - * @param catalogReader the {@link Prepare.CatalogReader} for finding tables/views referenced in - * the SQL statements - * @return a Substrait proto {@link io.substrait.proto.Plan} - * @throws SqlParseException if there is an error while parsing the SQL statements string - * @deprecated use {@link #convert(String, org.apache.calcite.prepare.Prepare.CatalogReader)} - * instead to get a {@link Plan} and convert that to a {@link io.substrait.proto.Plan} using - * {@link PlanProtoConverter#toProto(Plan)} - */ - @Deprecated - public io.substrait.proto.Plan execute(String sqlStatements, Prepare.CatalogReader catalogReader) - throws SqlParseException { - PlanProtoConverter planToProto = new PlanProtoConverter(); - return planToProto.toProto( - convert(sqlStatements, catalogReader, SqlDialect.DatabaseProduct.CALCITE.getDialect())); - } - /** * Converts one or more SQL statements into a Substrait {@link Plan}. * From e67a2997d24b0c5742ecf9f6c777cef0af3c6f75 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 10:17:55 -0800 Subject: [PATCH 09/28] feat: generate SqlOperatorTable from ConverterProvider --- .../substrait/isthmus/ConverterProvider.java | 10 ++++++++++ .../isthmus/DynamicConverterProvider.java | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 3b8926df2..505ddbee2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -2,6 +2,7 @@ import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.calcite.SubstraitOperatorTable; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.CallConverters; import io.substrait.isthmus.expression.FieldSelectionConverter; @@ -13,6 +14,7 @@ import java.util.List; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.sql.SqlOperatorTable; import org.apache.calcite.tools.RelBuilder; public class ConverterProvider { @@ -29,6 +31,10 @@ public ConverterProvider() { this(SubstraitTypeSystem.TYPE_FACTORY, DefaultExtensionCatalog.DEFAULT_COLLECTION); } + public ConverterProvider(SimpleExtension.ExtensionCollection extensions) { + this(SubstraitTypeSystem.TYPE_FACTORY, extensions); + } + public ConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { this( @@ -65,6 +71,10 @@ protected List getCallConverters() { return callConverters; } + protected SqlOperatorTable getSqlOperatorTable() { + return SubstraitOperatorTable.INSTANCE; + } + protected SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relBuilder) { return new SubstraitRelNodeConverter( typeFactory, diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 811292eba..9b2db08e7 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -1,6 +1,7 @@ package io.substrait.isthmus; import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.calcite.SubstraitOperatorTable; import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.ScalarFunctionConverter; import java.util.Collections; @@ -8,6 +9,8 @@ import java.util.stream.Collectors; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.SqlOperatorTable; +import org.apache.calcite.sql.util.SqlOperatorTables; public class DynamicConverterProvider extends ConverterProvider { @@ -43,6 +46,21 @@ protected List getCallConverters() { return callConverters; } + @Override + protected SqlOperatorTable getSqlOperatorTable() { + SimpleExtension.ExtensionCollection dynamicExtensionCollection = + ExtensionUtils.getDynamicExtensions(extensions); + if (!dynamicExtensionCollection.scalarFunctions().isEmpty() + || !dynamicExtensionCollection.aggregateFunctions().isEmpty()) { + List generatedDynamicOperators = + SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); + return SqlOperatorTables.chain( + SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedDynamicOperators)); + } + + return SubstraitOperatorTable.INSTANCE; + } + @Override public ScalarFunctionConverter getScalarFunctionConverter() { return scalarFunctionConverter; From c7d8a4bc82dbafa8ad4ba6840132b5a66375f2d4 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 10:20:15 -0800 Subject: [PATCH 10/28] feat: wire ConverterProvider into SqlToSubstrait --- .../io/substrait/isthmus/SqlToSubstrait.java | 36 +++++++------------ 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index b999fcf85..372463527 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -2,27 +2,23 @@ import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; -import io.substrait.isthmus.calcite.SubstraitOperatorTable; import io.substrait.isthmus.sql.SubstraitSqlToCalcite; import io.substrait.plan.ImmutablePlan.Builder; import io.substrait.plan.Plan; import io.substrait.plan.Plan.Version; -import io.substrait.plan.PlanProtoConverter; -import java.util.List; import org.apache.calcite.prepare.Prepare; import org.apache.calcite.sql.SqlDialect; -import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.SqlOperatorTable; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.sql.parser.SqlParser; -import org.apache.calcite.sql.util.SqlOperatorTables; /** Take a SQL statement and a set of table definitions and return a substrait plan. */ public class SqlToSubstrait extends SqlConverterBase { private final SqlOperatorTable operatorTable; + protected final ConverterProvider converterProvider; public SqlToSubstrait() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, null); + this(DefaultExtensionCatalog.DEFAULT_COLLECTION, FEATURES_DEFAULT); } public SqlToSubstrait(FeatureBoard features) { @@ -30,22 +26,16 @@ public SqlToSubstrait(FeatureBoard features) { } public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { - super(features, extensions); + this(extensions, new ConverterProvider(extensions), features); + } - if (featureBoard.allowDynamicUdfs()) { - SimpleExtension.ExtensionCollection dynamicExtensionCollection = - ExtensionUtils.getDynamicExtensions(extensions); - if (!dynamicExtensionCollection.scalarFunctions().isEmpty() - || !dynamicExtensionCollection.aggregateFunctions().isEmpty()) { - List generatedDynamicOperators = - SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, this.factory); - this.operatorTable = - SqlOperatorTables.chain( - SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedDynamicOperators)); - return; - } - } - this.operatorTable = SubstraitOperatorTable.INSTANCE; + public SqlToSubstrait( + SimpleExtension.ExtensionCollection extensions, + ConverterProvider converterProvider, + FeatureBoard features) { + super(features, extensions); + this.operatorTable = converterProvider.getSqlOperatorTable(); + this.converterProvider = converterProvider; } /** @@ -64,7 +54,7 @@ public Plan convert(final String sqlStatements, final Prepare.CatalogReader cata // TODO: consider case in which one sql passes conversion while others don't SubstraitSqlToCalcite.convertQueries(sqlStatements, catalogReader, operatorTable).stream() - .map(root -> SubstraitRelVisitor.convert(root, extensionCollection, featureBoard)) + .map(root -> SubstraitRelVisitor.convert(root, converterProvider)) .forEach(root -> builder.addRoots(root)); return builder.build(); @@ -92,7 +82,7 @@ public Plan convert( // TODO: consider case in which one sql passes conversion while others don't SubstraitSqlToCalcite.convertQueries(sqlStatements, catalogReader, sqlParserConfig).stream() - .map(root -> SubstraitRelVisitor.convert(root, extensionCollection, featureBoard)) + .map(root -> SubstraitRelVisitor.convert(root, converterProvider)) .forEach(root -> builder.addRoots(root)); return builder.build(); From b2f280d9dcddb1cd8a5c347b53ffbdbb01f54b22 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 10:46:00 -0800 Subject: [PATCH 11/28] test: updates scaffolding for UdfSqlToSubstraitTest --- .../substrait/isthmus/ConverterProvider.java | 2 +- .../isthmus/DynamicConverterProvider.java | 2 -- .../isthmus/SubstraitRelVisitor.java | 29 +++++++++++++++++++ .../io/substrait/isthmus/PlanTestBase.java | 25 +++++++++++----- .../isthmus/UdfSqlSubstraitTest.java | 1 + 5 files changed, 48 insertions(+), 11 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 505ddbee2..407ca646e 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -21,7 +21,7 @@ public class ConverterProvider { protected final RelDataTypeFactory typeFactory; - private final ScalarFunctionConverter scalarFunctionConverter; + protected ScalarFunctionConverter scalarFunctionConverter; private final AggregateFunctionConverter aggregateFunctionConverter; private final WindowFunctionConverter windowFunctionConverter; diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 9b2db08e7..05b6e891c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -16,8 +16,6 @@ public class DynamicConverterProvider extends ConverterProvider { private final SimpleExtension.ExtensionCollection extensions; - private final ScalarFunctionConverter scalarFunctionConverter; - public DynamicConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { super(typeFactory, extensions); diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 3253abd2a..126a284f8 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -615,6 +615,35 @@ public static Plan.Root convert(RelRoot relRoot, SimpleExtension.ExtensionCollec return convert(relRoot, extensions, FEATURES_DEFAULT); } + /** + * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using a custom visitor. + * + *

This is the main conversion entry point for a complete plan. It applies the provided {@link + * SubstraitRelVisitor} to the final projected {@link RelNode} from the {@code relRoot}, and wraps + * the resulting {@link Rel} in a {@link Plan.Root}. + * + *

This method also correctly extracts the final output field names, paying special attention + * to nested types (structs, maps) via the visitor's type converter, rather than using the names + * from {@code relRoot.validatedRowType} directly. + * + * @param relRoot The Calcite RelRoot to convert. This is expected to be a complete, optimized + * plan. + * @param visitor {@link SubstraitRelVisitor} or its subclass. This allows for custom visitor + * behavior. + * @return The resulting Substrait Plan.Root, containing the converted relational tree and the + * output names. + */ + public static Plan.Root convert(RelRoot relRoot, ConverterProvider converterProvider) { + SubstraitRelVisitor visitor = new SubstraitRelVisitor(converterProvider); + visitor.popFieldAccessDepthMap(relRoot.rel); + Rel rel = visitor.apply(relRoot.project()); + + // Avoid using the names from relRoot.validatedRowType because if there are + // nested types (i.e ROW, MAP, etc) the typeConverter will pad names correctly + List names = visitor.typeConverter.toNamedStruct(relRoot.validatedRowType).names(); + return Plan.Root.builder().input(rel).names(names).build(); + } + /** * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using a custom visitor. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index ca1e60c72..67d8e6ba4 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -54,6 +54,8 @@ public class PlanTestBase { protected static final CalciteCatalogReader TPCH_CATALOG; + protected ConverterProvider converterProvider; + static { try { String tpchCreateStatements = asString("tpch/schema.sql"); @@ -73,9 +75,15 @@ protected PlanTestBase() { } protected PlanTestBase(SimpleExtension.ExtensionCollection extensions) { + this(extensions, new ConverterProvider(extensions)); + } + + protected PlanTestBase( + SimpleExtension.ExtensionCollection extensions, ConverterProvider converterProvider) { this.extensions = extensions; this.sb = new SubstraitBuilder(extensions); - this.substraitToCalcite = new SubstraitToCalcite(extensions, typeFactory); + this.substraitToCalcite = new SubstraitToCalcite(converterProvider); + this.converterProvider = converterProvider; } public static String asString(String resource) throws IOException { @@ -139,7 +147,8 @@ protected RelRoot assertSqlSubstraitRelRoundTrip( // Return list of sql -> Substrait rel -> Calcite rel. SqlToSubstrait s2s = new SqlToSubstrait(); - SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(extensions, typeFactory); + SubstraitToCalcite substraitToCalcite = + new SubstraitToCalcite(converterProvider, catalogReader); // 1. SQL -> Substrait Plan Plan plan1 = s2s.convert(query, catalogReader); @@ -151,7 +160,7 @@ protected RelRoot assertSqlSubstraitRelRoundTrip( RelRoot relRoot2 = substraitToCalcite.convert(pojo1); // 4. Calcite RelNode -> Substrait Rel - Plan.Root pojo2 = SubstraitRelVisitor.convert(relRoot2, extensions); + Plan.Root pojo2 = SubstraitRelVisitor.convert(relRoot2, converterProvider); assertEquals(pojo1, pojo2); return relRoot2; @@ -186,11 +195,11 @@ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( featureBoard != null ? featureBoard : ImmutableFeatureBoard.builder().build(); SubstraitToCalcite substraitToCalcite = - new SubstraitToCalcite(extensions, typeFactory, TypeConverter.DEFAULT, null, features); - SqlToSubstrait s = new SqlToSubstrait(extensions, features); + new SubstraitToCalcite(converterProvider, catalogReader); + SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(extensions, converterProvider, featureBoard); // 1. SQL -> Substrait Plan - Plan plan1 = s.convert(query, catalogReader); + Plan plan1 = sqlToSubstrait.convert(query, catalogReader); // 2. Substrait Plan -> Substrait Root (POJO 1) Plan.Root pojo1 = plan1.getRoots().get(0); @@ -199,7 +208,7 @@ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( RelRoot relRoot2 = substraitToCalcite.convert(pojo1); // 4. Calcite RelNode -> Substrait Root (POJO 2) - Plan.Root pojo2 = SubstraitRelVisitor.convert(relRoot2, extensions, features); + Plan.Root pojo2 = SubstraitRelVisitor.convert(relRoot2, converterProvider); // Note: pojo1 and pojo2 may differ due to different optimization strategies applied by: // - SqlNode->RelRoot conversion during SQL->Substrait conversion @@ -210,7 +219,7 @@ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( RelRoot relRoot3 = substraitToCalcite.convert(pojo2); // 6. Calcite RelNode -> Substrait Root (POJO 3) - Plan.Root pojo3 = SubstraitRelVisitor.convert(relRoot3, extensions, features); + Plan.Root pojo3 = SubstraitRelVisitor.convert(relRoot3, converterProvider); // Verify that subsequent round trips are stable (pojo2 and pojo3 should be identical) assertEquals(pojo2, pojo3); diff --git a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java index 69b8be3b9..5eea0f9fc 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java @@ -13,6 +13,7 @@ class UdfSqlSubstraitTest extends PlanTestBase { UdfSqlSubstraitTest() { super(loadExtensions(List.of(CUSTOM_FUNCTION_PATH))); + this.converterProvider = new DynamicConverterProvider(typeFactory, extensions); } @Test From 3370f590c0bd272942f0d01814cc0ac09efb4324 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 11:29:52 -0800 Subject: [PATCH 12/28] refactor: remove allowDynamicUdfs from FeatureBoard --- .../io/substrait/isthmus/FeatureBoard.java | 15 ------------- .../io/substrait/isthmus/PlanTestBase.java | 22 +++---------------- .../isthmus/UdfSqlSubstraitTest.java | 10 ++++----- 3 files changed, 7 insertions(+), 40 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java b/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java index a54f24146..8db29f73c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java +++ b/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java @@ -17,19 +17,4 @@ public abstract class FeatureBoard { public Casing unquotedCasing() { return Casing.TO_UPPER; } - - /** - * Controls whether to support dynamic user-defined functions (UDFs) during SQL to Substrait plan - * conversion. - * - *

When enabled, custom functions defined in extension YAML files are available for use in SQL - * queries. These functions will be dynamically converted to SQL operators during plan conversion. - * This feature must be explicitly enabled by users and is disabled by default. - * - * @return true if dynamic UDFs should be supported; false otherwise (default) - */ - @Value.Default - public boolean allowDynamicUdfs() { - return false; - } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index 67d8e6ba4..e4d779326 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -184,19 +184,13 @@ protected RelRoot assertSqlSubstraitRelRoundTrip( * * @param query the SQL query to test * @param catalogReader the Calcite catalog with table definitions - * @param featureBoard optional FeatureBoard to control conversion behavior (e.g., dynamic UDFs). - * If null, a default FeatureBoard is used. */ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( - String query, Prepare.CatalogReader catalogReader, FeatureBoard featureBoard) - throws Exception { - // Use provided FeatureBoard, or create default if null - FeatureBoard features = - featureBoard != null ? featureBoard : ImmutableFeatureBoard.builder().build(); - + String query, Prepare.CatalogReader catalogReader) throws Exception { + FeatureBoard features = ImmutableFeatureBoard.builder().build(); SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider, catalogReader); - SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(extensions, converterProvider, featureBoard); + SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(extensions, converterProvider, features); // 1. SQL -> Substrait Plan Plan plan1 = sqlToSubstrait.convert(query, catalogReader); @@ -226,16 +220,6 @@ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( return relRoot2; } - /** - * Convenience overload of {@link #assertSqlSubstraitRelRoundTripLoosePojoComparison(String, - * Prepare.CatalogReader, FeatureBoard)} with default FeatureBoard behavior (no dynamic UDFs). - */ - protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( - String query, Prepare.CatalogReader catalogReader) throws Exception { - return assertSqlSubstraitRelRoundTripLoosePojoComparison( - query, catalogReader, ImmutableFeatureBoard.builder().build()); - } - @Beta protected void assertFullRoundTrip(String query) throws SqlParseException { assertFullRoundTrip(query, TPCH_CATALOG); diff --git a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java index 5eea0f9fc..3bab99bd5 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java @@ -23,16 +23,14 @@ void customUdfTest() throws Exception { SubstraitCreateStatementParser.processCreateStatementsToCatalog( "CREATE TABLE t(x VARCHAR NOT NULL)"); - FeatureBoard featureBoard = ImmutableFeatureBoard.builder().allowDynamicUdfs(true).build(); - assertSqlSubstraitRelRoundTripLoosePojoComparison( - "SELECT regexp_extract_custom(x, 'ab') from t", catalogReader, featureBoard); + "SELECT regexp_extract_custom(x, 'ab') from t", catalogReader); assertSqlSubstraitRelRoundTripLoosePojoComparison( - "SELECT format_text('UPPER', x) FROM t", catalogReader, featureBoard); + "SELECT format_text('UPPER', x) FROM t", catalogReader); assertSqlSubstraitRelRoundTripLoosePojoComparison( - "SELECT system_property_get(x) FROM t", catalogReader, featureBoard); + "SELECT system_property_get(x) FROM t", catalogReader); assertSqlSubstraitRelRoundTripLoosePojoComparison( - "SELECT safe_divide_custom(10,0) FROM t", catalogReader, featureBoard); + "SELECT safe_divide_custom(10,0) FROM t", catalogReader); } private static SimpleExtension.ExtensionCollection loadExtensions( From 77bb7a5e169a271a64d6eb9276611a146997fe68 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 17:31:48 -0800 Subject: [PATCH 13/28] test: boring test updates --- .../io/substrait/isthmus/RelExtensionRoundtripTest.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java index 0cd785379..537cc6988 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java @@ -81,7 +81,9 @@ void roundtrip(Rel pojo1) { Context.newContext()); // Calcite -> Substrait POJO 3 - Rel pojo3 = (new CustomSubstraitRelVisitor(typeFactory, extensions)).apply(calcite); + Rel pojo3 = + (new CustomSubstraitRelVisitor(new ConverterProvider(typeFactory, extensions))) + .apply(calcite); assertEquals(pojo1, pojo3); } @@ -246,9 +248,8 @@ public RelNode visit(ExtensionMulti extensionMulti, Context context) throws Runt /** Extends the standard {@link SubstraitRelVisitor} to handle the {@link ColumnAppenderRel} */ static class CustomSubstraitRelVisitor extends SubstraitRelVisitor { - public CustomSubstraitRelVisitor( - RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { - super(typeFactory, extensions); + public CustomSubstraitRelVisitor(ConverterProvider converterProvider) { + super(converterProvider); } @Override From 32aca85f05a1025311648eaad1b594fc97afb0e7 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 17:32:20 -0800 Subject: [PATCH 14/28] test: more interesting test updates --- .../substrait/isthmus/CustomFunctionTest.java | 25 +++++++++++++------ .../io/substrait/isthmus/PlanTestBase.java | 19 +++++++------- .../isthmus/ProtoPlanConverterTest.java | 5 ++-- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java index 79633775b..f445c9338 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java @@ -6,6 +6,7 @@ import com.google.protobuf.Any; import io.substrait.expression.Expression.UserDefinedLiteral; import io.substrait.expression.ExpressionCreator; +import io.substrait.extension.ExtensionCollector; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.FunctionMappings; @@ -14,7 +15,9 @@ import io.substrait.isthmus.utils.UserTypeFactory; import io.substrait.proto.Expression; import io.substrait.proto.Expression.Literal.Builder; +import io.substrait.relation.ProtoRelConverter; import io.substrait.relation.Rel; +import io.substrait.relation.RelProtoConverter; import io.substrait.type.Type; import io.substrait.type.TypeCreator; import java.io.IOException; @@ -49,8 +52,8 @@ class CustomFunctionTest extends PlanTestBase { } // Load custom extension into an ExtensionCollection - static final SimpleExtension.ExtensionCollection extensionCollection = - SimpleExtension.load("custom.yaml", FUNCTIONS_CUSTOM); + static final SimpleExtension.ExtensionCollection CUSTOM_EXTENSIONS = + SimpleExtension.load(URN, FUNCTIONS_CUSTOM); // Create user-defined types static final String aTypeName = "a_type"; @@ -231,18 +234,18 @@ public RelDataType toCalcite(Type.UserDefined type) { // Create Function Converters that can handle the custom functions ScalarFunctionConverter scalarFunctionConverter = new ScalarFunctionConverter( - extensionCollection.scalarFunctions(), + CUSTOM_EXTENSIONS.scalarFunctions(), additionalScalarSignatures, typeFactory, typeConverter); AggregateFunctionConverter aggregateFunctionConverter = new AggregateFunctionConverter( - extensionCollection.aggregateFunctions(), + CUSTOM_EXTENSIONS.aggregateFunctions(), additionalAggregateSignatures, typeFactory, typeConverter); WindowFunctionConverter windowFunctionConverter = - new WindowFunctionConverter(extensionCollection.windowFunctions(), typeFactory); + new WindowFunctionConverter(CUSTOM_EXTENSIONS.windowFunctions(), typeFactory); ConverterProvider converterProvider = new ConverterProvider( @@ -252,10 +255,13 @@ public RelDataType toCalcite(Type.UserDefined type) { windowFunctionConverter, typeConverter); - final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider); - // Create a SubstraitRelVisitor that uses the custom Function Converters final SubstraitRelVisitor calciteToSubstrait = new SubstraitRelVisitor(converterProvider); + final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider); + + CustomFunctionTest() { + super(CUSTOM_EXTENSIONS); + } @Test void customScalarFunctionRoundtrip() { @@ -573,6 +579,11 @@ void customTypesLiteralInFunctionsRoundtrip() { RelNode calciteRel = substraitToCalcite.convert(rel1); Rel rel2 = calciteToSubstrait.apply(calciteRel); assertEquals(rel1, rel2); + + ExtensionCollector extensionCollector = new ExtensionCollector(); + io.substrait.proto.Rel protoRel = new RelProtoConverter(extensionCollector).toProto(rel1); + Rel rel3 = new ProtoRelConverter(extensionCollector, CUSTOM_EXTENSIONS).from(protoRel); + assertEquals(rel1, rel3); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index e4d779326..49e986c85 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -52,10 +52,10 @@ public class PlanTestBase { protected final SubstraitBuilder sb; protected final SubstraitToCalcite substraitToCalcite; - protected static final CalciteCatalogReader TPCH_CATALOG; - protected ConverterProvider converterProvider; + protected static final CalciteCatalogReader TPCH_CATALOG; + static { try { String tpchCreateStatements = asString("tpch/schema.sql"); @@ -82,8 +82,8 @@ protected PlanTestBase( SimpleExtension.ExtensionCollection extensions, ConverterProvider converterProvider) { this.extensions = extensions; this.sb = new SubstraitBuilder(extensions); - this.substraitToCalcite = new SubstraitToCalcite(converterProvider); this.converterProvider = converterProvider; + this.substraitToCalcite = new SubstraitToCalcite(converterProvider); } public static String asString(String resource) throws IOException { @@ -187,10 +187,9 @@ protected RelRoot assertSqlSubstraitRelRoundTrip( */ protected RelRoot assertSqlSubstraitRelRoundTripLoosePojoComparison( String query, Prepare.CatalogReader catalogReader) throws Exception { - FeatureBoard features = ImmutableFeatureBoard.builder().build(); SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider, catalogReader); - SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(extensions, converterProvider, features); + SqlToSubstrait sqlToSubstrait = new SqlToSubstrait(converterProvider); // 1. SQL -> Substrait Plan Plan plan1 = sqlToSubstrait.convert(query, catalogReader); @@ -267,7 +266,7 @@ protected void assertFullRoundTrip(String sqlQuery, Prepare.CatalogReader catalo // Substrait Root 2 -> Calcite 2 final SubstraitToCalcite substraitToCalcite = - new SubstraitToCalcite(extensions, typeFactory, catalogReader); + new SubstraitToCalcite(converterProvider, catalogReader); RelRoot calcite2 = substraitToCalcite.convert(root2); // It would be ideal to compare calcite1 and calcite2, however there isn't a good mechanism to @@ -325,7 +324,7 @@ protected void assertFullRoundTripWithIdentityProjectionWorkaround( assertEquals(root0, root1); final SubstraitToCalcite substraitToCalcite = - new SubstraitToCalcite(extensions, typeFactory, catalogReader); + new SubstraitToCalcite(converterProvider, catalogReader); // Substrait POJO 1 -> Calcite 1 RelRoot calcite1 = substraitToCalcite.convert(root1); @@ -373,7 +372,7 @@ protected void assertFullRoundTrip(Rel pojo1) { assertEquals(pojo1, pojo2); // Substrait POJO 2 -> Calcite - RelNode calcite = new SubstraitToCalcite(extensions, typeFactory).convert(pojo2); + RelNode calcite = new SubstraitToCalcite(converterProvider).convert(pojo2); // Calcite -> Substrait POJO 3 io.substrait.relation.Rel pojo3 = SubstraitRelVisitor.convert(calcite, extensions); @@ -404,7 +403,7 @@ protected void assertFullRoundTrip(Plan.Root pojo1) { assertEquals(pojo1, pojo2); // Substrait POJO 2 -> Calcite - RelRoot calcite = new SubstraitToCalcite(extensions, typeFactory).convert(pojo2); + RelRoot calcite = new SubstraitToCalcite(converterProvider).convert(pojo2); // Calcite -> Substrait POJO 3 io.substrait.plan.Plan.Root pojo3 = SubstraitRelVisitor.convert(calcite, extensions); @@ -439,7 +438,7 @@ protected String toSql(Plan plan) { assertEquals(1, roots.size(), "number of roots"); Root root = roots.get(0); - RelRoot relRoot = new SubstraitToCalcite(extensions, typeFactory).convert(root); + RelRoot relRoot = new SubstraitToCalcite(converterProvider).convert(root); RelNode project = relRoot.project(true); return SubstraitSqlDialect.toSql(project).getSql(); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/ProtoPlanConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/ProtoPlanConverterTest.java index da8423c03..f71a81c1a 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/ProtoPlanConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/ProtoPlanConverterTest.java @@ -73,7 +73,6 @@ public Optional visit(Cross cross, EmptyVisitationContext context) return super.visit(cross, context); } }; - ImmutableFeatureBoard featureBoard = ImmutableFeatureBoard.builder().build(); String query1 = "select\n" @@ -82,7 +81,7 @@ public Optional visit(Cross cross, EmptyVisitationContext context) + "from\n" + " \"customer\" c cross join\n" + " \"orders\" o"; - Plan plan1 = assertProtoPlanRoundrip(query1, new SqlToSubstrait(featureBoard)); + Plan plan1 = assertProtoPlanRoundrip(query1, new SqlToSubstrait()); plan1 .getRoots() .forEach( @@ -96,7 +95,7 @@ public Optional visit(Cross cross, EmptyVisitationContext context) + "from\n" + " \"customer\" c,\n" + " \"orders\" o"; - Plan plan2 = assertProtoPlanRoundrip(query2, new SqlToSubstrait(featureBoard)); + Plan plan2 = assertProtoPlanRoundrip(query2, new SqlToSubstrait()); plan2 .getRoots() .forEach( From 717fd5544c0947ff06770be81054616ec4cf1278 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 22 Dec 2025 18:52:49 -0800 Subject: [PATCH 15/28] docs: document new ConverterProvider base functionality --- .../isthmus/cli/IsthmusEntryPoint.java | 19 +- .../substrait/isthmus/ConverterProvider.java | 171 ++++++++++++++++-- .../substrait/isthmus/SqlConverterBase.java | 33 +--- .../isthmus/SqlExpressionToSubstrait.java | 17 +- .../io/substrait/isthmus/SqlToSubstrait.java | 25 ++- .../isthmus/SubstraitRelNodeConverter.java | 96 +--------- .../isthmus/SubstraitRelVisitor.java | 150 ++++----------- .../substrait/isthmus/SubstraitToCalcite.java | 75 +------- .../io/substrait/isthmus/SubstraitToSql.java | 19 +- 9 files changed, 245 insertions(+), 360 deletions(-) diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index 5cd446b32..a700cc0ff 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -1,12 +1,9 @@ package io.substrait.isthmus.cli; -import com.google.common.annotations.VisibleForTesting; import com.google.protobuf.Message; import com.google.protobuf.TextFormat; import com.google.protobuf.util.JsonFormat; import io.substrait.extension.DefaultExtensionCatalog; -import io.substrait.isthmus.FeatureBoard; -import io.substrait.isthmus.ImmutableFeatureBoard; import io.substrait.isthmus.SqlExpressionToSubstrait; import io.substrait.isthmus.SqlToSubstrait; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -16,7 +13,6 @@ import java.io.IOException; import java.util.List; import java.util.concurrent.Callable; -import org.apache.calcite.avatica.util.Casing; import org.apache.calcite.prepare.Prepare; import picocli.CommandLine; import picocli.CommandLine.Command; @@ -57,11 +53,6 @@ enum OutputFormat { BINARY, // protobuf BINARY format } - @Option( - names = {"--unquotedcasing"}, - description = "Calcite's casing policy for unquoted identifiers: ${COMPLETION-CANDIDATES}") - private Casing unquotedCasing = Casing.TO_UPPER; - /** * Standard Java Main method invoked by the isthmus CLI command. * @@ -89,15 +80,14 @@ public static void main(String... args) { @Override public Integer call() throws Exception { - FeatureBoard featureBoard = buildFeatureBoard(); // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { SqlExpressionToSubstrait converter = - new SqlExpressionToSubstrait(featureBoard, DefaultExtensionCatalog.DEFAULT_COLLECTION); + new SqlExpressionToSubstrait(DefaultExtensionCatalog.DEFAULT_COLLECTION); ExtendedExpression extendedExpression = converter.convert(sqlExpressions, createStatements); printMessage(extendedExpression); } else { // by default Isthmus image are parsing SQL Query - SqlToSubstrait converter = new SqlToSubstrait(featureBoard); + SqlToSubstrait converter = new SqlToSubstrait(); Prepare.CatalogReader catalog = SubstraitCreateStatementParser.processCreateStatementsToCatalog( createStatements.toArray(String[]::new)); @@ -116,9 +106,4 @@ private void printMessage(Message message) throws IOException { message.writeTo(System.out); } } - - @VisibleForTesting - FeatureBoard buildFeatureBoard() { - return ImmutableFeatureBoard.builder().unquotedCasing(unquotedCasing).build(); - } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 407ca646e..5734a4b68 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -5,27 +5,54 @@ import io.substrait.isthmus.calcite.SubstraitOperatorTable; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.CallConverters; +import io.substrait.isthmus.expression.ExpressionRexConverter; import io.substrait.isthmus.expression.FieldSelectionConverter; +import io.substrait.isthmus.expression.RexExpressionConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; import io.substrait.isthmus.expression.SqlArrayValueConstructorCallConverter; import io.substrait.isthmus.expression.SqlMapValueConstructorCallConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; +import io.substrait.relation.Rel; import java.util.ArrayList; import java.util.List; +import java.util.function.Function; +import org.apache.calcite.avatica.util.Casing; +import org.apache.calcite.config.CalciteConnectionConfig; +import org.apache.calcite.config.CalciteConnectionProperty; +import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.prepare.Prepare; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.sql.SqlOperatorTable; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl; +import org.apache.calcite.sql.validate.SqlConformanceEnum; +import org.apache.calcite.sql2rel.SqlToRelConverter; +import org.apache.calcite.tools.Frameworks; import org.apache.calcite.tools.RelBuilder; +/** + * ConverterProvider provides a single-point of configuration for a number of conversions: {@code + * SQl <-> Calcite <-> Substrait} + * + *

It is consumed by all conversion classes as their primary source of configuration. + * + *

The no argument constructor {@link #ConverterProvider()} provides reasonable system defaults. + * + *

Other constructors allow for further customization of conversion behaviours. + * + *

More in-depth customization can be achieved by extending this class, as is done in {@link + * DynamicConverterProvider}. + */ public class ConverterProvider { - protected final RelDataTypeFactory typeFactory; + protected RelDataTypeFactory typeFactory; protected ScalarFunctionConverter scalarFunctionConverter; - private final AggregateFunctionConverter aggregateFunctionConverter; - private final WindowFunctionConverter windowFunctionConverter; + protected AggregateFunctionConverter aggregateFunctionConverter; + protected WindowFunctionConverter windowFunctionConverter; - private final TypeConverter typeConverter; + protected TypeConverter typeConverter; public ConverterProvider() { this(SubstraitTypeSystem.TYPE_FACTORY, DefaultExtensionCatalog.DEFAULT_COLLECTION); @@ -58,6 +85,90 @@ public ConverterProvider( this.typeConverter = tc; } + // SQL to Calcite Processing + + /** + * {@link SqlParser.Config} is a Calcite class which controls SQL parsing behaviour like + * identifier casing. + */ + protected SqlParser.Config getSqlParserConfig() { + return SqlParser.Config.DEFAULT + .withUnquotedCasing(Casing.TO_UPPER) + .withParserFactory(SqlDdlParserImpl.FACTORY) + .withConformance(SqlConformanceEnum.LENIENT); + } + + /** + * {@link CalciteConnectionConfig} is a Calcite class which controls SQL processing behaviour like + * table name case-sensitivity. + */ + protected CalciteConnectionConfig getCalciteConnectionConfig() { + return CalciteConnectionConfig.DEFAULT.set(CalciteConnectionProperty.CASE_SENSITIVE, "false"); + } + + /** + * {@link SqlToRelConverter.Config} is a Calcite class which controls SQL processing behaviour + * like field-trimming. + */ + protected SqlToRelConverter.Config getSqlToRelConverterConfig() { + return SqlToRelConverter.config().withTrimUnusedFields(true).withExpand(false); + } + + /** + * {@link SqlOperatorTable} is a Calcite class which stores the {@link + * org.apache.calcite.sql.SqlOperator}s available and controls valid identifiers during SQL + * processing. + */ + protected SqlOperatorTable getSqlOperatorTable() { + return SubstraitOperatorTable.INSTANCE; + } + + // Substrait to Calcite Processing + + /** + * {@link SubstraitToCalcite} is an Isthmus class for converting a Substrait {@link Rel} or {@link + * io.substrait.plan.Plan.Root} to a Calcite {@link org.apache.calcite.rel.RelNode} or {@link + * org.apache.calcite.rel.RelRoot} + */ + protected SubstraitToCalcite getSubstraitToCalcite() { + return new SubstraitToCalcite(this); + } + + /** + * {@link SubstraitToCalcite} is an Isthmus class for converting a Substrait {@link Rel} or {@link + * io.substrait.plan.Plan.Root} to a Calcite {@link org.apache.calcite.rel.RelNode} or {@link + * org.apache.calcite.rel.RelRoot} + * + * @param catalogReader a Calcite {@link Prepare.CatalogReader} used to construct a {@link + * RelBuilder} for use in creating Calcite {@link org.apache.calcite.rel.RelNode}s + */ + protected SubstraitToCalcite getSubstraitToCalcite(Prepare.CatalogReader catalogReader) { + return new SubstraitToCalcite(this, catalogReader); + } + + // Calcite to Substrait Processing + + /** + * A {@link SubstraitRelVisitor} converts Calcite {@link org.apache.calcite.rel.RelNode}s to + * Substrait {@link Rel}s + */ + protected SubstraitRelVisitor getSubstraitRelVisitor() { + return new SubstraitRelVisitor(this); + } + + /** + * A {@link RexExpressionConverter} converts Calcite {@link org.apache.calcite.rex.RexNode}s to + * Substrait equivalents. + */ + protected RexExpressionConverter getRexExpressionConverter(SubstraitRelVisitor srv) { + return new RexExpressionConverter( + srv, getCallConverters(), getWindowFunctionConverter(), getTypeConverter()); + } + + /** + * {@link CallConverter}s are used to convert Calcite {@link org.apache.calcite.rex.RexCall}s to + * Substrait equivalents. + */ protected List getCallConverters() { ArrayList callConverters = new ArrayList<>(); callConverters.add(new FieldSelectionConverter(typeConverter)); @@ -71,20 +182,54 @@ protected List getCallConverters() { return callConverters; } - protected SqlOperatorTable getSqlOperatorTable() { - return SubstraitOperatorTable.INSTANCE; + // Substrait To Calcite Processing + + /** + * When converting from Substrait to Calcite, Calcite needs to have a schema available. The + * default strategy uses a {@link SchemaCollector} to generate a {@link CalciteSchema} on the fly + * based on the leaf nodes of a Substrait plan. + * + *

Override to customize the schema generation behaviour + */ + protected Function getSchemaResolver() { + SchemaCollector schemaCollector = new SchemaCollector(this); + return schemaCollector::toSchema; } + /** + * A {@link SubstraitRelNodeConverter} is used when converting from Substrait {@link Rel}s to + * Calcite {@link org.apache.calcite.rel.RelNode}s. + */ protected SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relBuilder) { - return new SubstraitRelNodeConverter( - typeFactory, - relBuilder, - getScalarFunctionConverter(), - getAggregateFunctionConverter(), - getWindowFunctionConverter(), - typeConverter); + return new SubstraitRelNodeConverter(relBuilder, this); + } + + /** + * A {@link ExpressionRexConverter} converts Substrait {@link io.substrait.expression.Expression} + * to Calcite equivalents + */ + protected ExpressionRexConverter getExpressionRexConverter( + SubstraitRelNodeConverter relNodeConverter) { + ExpressionRexConverter erc = + new ExpressionRexConverter( + getTypeFactory(), + getScalarFunctionConverter(), + getWindowFunctionConverter(), + getTypeConverter()); + erc.setRelNodeConverter(relNodeConverter); + return erc; } + /** + * A {@link RelBuilder} is a Calcite class used as a factory for creating {@link + * org.apache.calcite.rel.RelNode}s. + */ + protected RelBuilder getRelBuilder(CalciteSchema schema) { + return RelBuilder.create(Frameworks.newConfigBuilder().defaultSchema(schema.plus()).build()); + } + + // Utility Getters + public RelDataTypeFactory getTypeFactory() { return typeFactory; } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlConverterBase.java b/isthmus/src/main/java/io/substrait/isthmus/SqlConverterBase.java index f667deab0..170dc4f94 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlConverterBase.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlConverterBase.java @@ -1,7 +1,5 @@ package io.substrait.isthmus; -import io.substrait.extension.DefaultExtensionCatalog; -import io.substrait.extension.SimpleExtension; import org.apache.calcite.config.CalciteConnectionConfig; import org.apache.calcite.config.CalciteConnectionProperty; import org.apache.calcite.plan.Contexts; @@ -14,12 +12,10 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.sql.parser.SqlParser; -import org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl; -import org.apache.calcite.sql.validate.SqlConformanceEnum; import org.apache.calcite.sql2rel.SqlToRelConverter; public class SqlConverterBase { - protected final SimpleExtension.ExtensionCollection extensionCollection; + protected final ConverterProvider converterProvider; public static final CalciteConnectionConfig CONNECTION_CONFIG = CalciteConnectionConfig.DEFAULT.set( @@ -32,15 +28,11 @@ public class SqlConverterBase { final SqlParser.Config parserConfig; - protected static final FeatureBoard FEATURES_DEFAULT = ImmutableFeatureBoard.builder().build(); - final FeatureBoard featureBoard; - - protected SqlConverterBase( - FeatureBoard features, SimpleExtension.ExtensionCollection extensionCollection) { - this.factory = SubstraitTypeSystem.TYPE_FACTORY; - this.config = - CalciteConnectionConfig.DEFAULT.set(CalciteConnectionProperty.CASE_SENSITIVE, "false"); - this.converterConfig = SqlToRelConverter.config().withTrimUnusedFields(true).withExpand(false); + protected SqlConverterBase(ConverterProvider converterProvider) { + this.converterProvider = converterProvider; + this.factory = converterProvider.getTypeFactory(); + this.config = converterProvider.getCalciteConnectionConfig(); + this.converterConfig = converterProvider.getSqlToRelConverterConfig(); VolcanoPlanner planner = new VolcanoPlanner(RelOptCostImpl.FACTORY, Contexts.of("hello")); this.relOptCluster = RelOptCluster.create(planner, new RexBuilder(factory)); relOptCluster.setMetadataQuerySupplier( @@ -49,17 +41,6 @@ protected SqlConverterBase( new ProxyingMetadataHandlerProvider(DefaultRelMetadataProvider.INSTANCE); return new RelMetadataQuery(handler); }); - featureBoard = features == null ? FEATURES_DEFAULT : features; - parserConfig = - SqlParser.Config.DEFAULT - .withUnquotedCasing(featureBoard.unquotedCasing()) - .withParserFactory(SqlDdlParserImpl.FACTORY) - .withConformance(SqlConformanceEnum.LENIENT); - - this.extensionCollection = extensionCollection; - } - - protected SqlConverterBase(FeatureBoard features) { - this(features, DefaultExtensionCatalog.DEFAULT_COLLECTION); + parserConfig = converterProvider.getSqlParserConfig(); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java index 3d45f8bde..3bdc2e56c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java @@ -7,7 +7,6 @@ import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.calcite.SubstraitTable; import io.substrait.isthmus.expression.RexExpressionConverter; -import io.substrait.isthmus.expression.ScalarFunctionConverter; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; import io.substrait.isthmus.sql.SubstraitSqlValidator; import io.substrait.type.NamedStruct; @@ -35,15 +34,17 @@ public class SqlExpressionToSubstrait extends SqlConverterBase { protected final RexExpressionConverter rexConverter; public SqlExpressionToSubstrait() { - this(FEATURES_DEFAULT, DefaultExtensionCatalog.DEFAULT_COLLECTION); + this(DefaultExtensionCatalog.DEFAULT_COLLECTION); } - public SqlExpressionToSubstrait( - FeatureBoard features, SimpleExtension.ExtensionCollection extensions) { - super(features, extensions); - ScalarFunctionConverter scalarFunctionConverter = - new ScalarFunctionConverter(extensions.scalarFunctions(), factory); - this.rexConverter = new RexExpressionConverter(scalarFunctionConverter); + /** Use {@link #SqlExpressionToSubstrait(ConverterProvider)} instead */ + public SqlExpressionToSubstrait(SimpleExtension.ExtensionCollection extensions) { + this(new ConverterProvider(extensions)); + } + + public SqlExpressionToSubstrait(ConverterProvider converterProvider) { + super(converterProvider); + this.rexConverter = new RexExpressionConverter(converterProvider.getScalarFunctionConverter()); } private static final class Result { diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index 372463527..0a61986f9 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -12,28 +12,27 @@ import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.sql.parser.SqlParser; -/** Take a SQL statement and a set of table definitions and return a substrait plan. */ +/** + * Take a SQL statement and a set of table definitions and return a substrait plan. + * + *

Conversion behaviours can be customized using a {@link ConverterProvider} + */ public class SqlToSubstrait extends SqlConverterBase { private final SqlOperatorTable operatorTable; protected final ConverterProvider converterProvider; public SqlToSubstrait() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, FEATURES_DEFAULT); + this(DefaultExtensionCatalog.DEFAULT_COLLECTION); } - public SqlToSubstrait(FeatureBoard features) { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION, features); + /** Use {@link SqlToSubstrait#SqlToSubstrait(ConverterProvider)} instead */ + @Deprecated + public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions) { + this(new ConverterProvider(extensions)); } - public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { - this(extensions, new ConverterProvider(extensions), features); - } - - public SqlToSubstrait( - SimpleExtension.ExtensionCollection extensions, - ConverterProvider converterProvider, - FeatureBoard features) { - super(features, extensions); + public SqlToSubstrait(ConverterProvider converterProvider) { + super(converterProvider); this.operatorTable = converterProvider.getSqlOperatorTable(); this.converterProvider = converterProvider; } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 66b221c8c..0920cabd3 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -13,7 +13,6 @@ import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.ExpressionRexConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; -import io.substrait.isthmus.expression.WindowFunctionConverter; import io.substrait.relation.AbstractDdlRel; import io.substrait.relation.AbstractRelVisitor; import io.substrait.relation.AbstractUpdate; @@ -55,7 +54,6 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; -import org.apache.calcite.plan.RelOptCluster; import org.apache.calcite.plan.RelOptTable; import org.apache.calcite.plan.RelTraitDef; import org.apache.calcite.prepare.Prepare; @@ -82,7 +80,6 @@ import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlStdOperatorTable; -import org.apache.calcite.sql.parser.SqlParser; import org.apache.calcite.tools.Frameworks; import org.apache.calcite.tools.RelBuilder; @@ -103,114 +100,37 @@ public class SubstraitRelNodeConverter protected final RexBuilder rexBuilder; private final TypeConverter typeConverter; + /** Use {@link #SubstraitRelNodeConverter(RelBuilder, ConverterProvider)} instead */ + @Deprecated public SubstraitRelNodeConverter( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory, RelBuilder relBuilder) { - this(extensions, typeFactory, relBuilder, ImmutableFeatureBoard.builder().build()); + this(relBuilder, new ConverterProvider(typeFactory, extensions)); } - public SubstraitRelNodeConverter( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - RelBuilder relBuilder, - FeatureBoard featureBoard) { - this( - typeFactory, - relBuilder, - new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), - new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory), - new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), - TypeConverter.DEFAULT); - } - - public SubstraitRelNodeConverter( - RelDataTypeFactory typeFactory, - RelBuilder relBuilder, - ScalarFunctionConverter scalarFunctionConverter, - AggregateFunctionConverter aggregateFunctionConverter, - WindowFunctionConverter windowFunctionConverter, - TypeConverter typeConverter) { - this( - relBuilder, - new ExpressionRexConverter( - typeFactory, scalarFunctionConverter, windowFunctionConverter, typeConverter), - new ConverterProvider( - typeFactory, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter)); - } - - public SubstraitRelNodeConverter( - RelDataTypeFactory typeFactory, - RelBuilder relBuilder, - ScalarFunctionConverter scalarFunctionConverter, - AggregateFunctionConverter aggregateFunctionConverter, - WindowFunctionConverter windowFunctionConverter, - TypeConverter typeConverter, - ExpressionRexConverter expressionRexConverter) { - this( - relBuilder, - expressionRexConverter, - new ConverterProvider( - typeFactory, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter)); - } - - public SubstraitRelNodeConverter( - RelBuilder relBuilder, - ExpressionRexConverter expressionRexConverter, - ConverterProvider converterProvider) { + public SubstraitRelNodeConverter(RelBuilder relBuilder, ConverterProvider converterProvider) { this.typeFactory = converterProvider.getTypeFactory(); this.typeConverter = converterProvider.getTypeConverter(); this.relBuilder = relBuilder; this.rexBuilder = new RexBuilder(typeFactory); this.scalarFunctionConverter = converterProvider.getScalarFunctionConverter(); this.aggregateFunctionConverter = converterProvider.getAggregateFunctionConverter(); - this.expressionRexConverter = expressionRexConverter; - this.expressionRexConverter.setRelNodeConverter(this); - } - - public static RelNode convert( - Rel relRoot, - RelOptCluster relOptCluster, - Prepare.CatalogReader catalogReader, - SqlParser.Config parserConfig, - SimpleExtension.ExtensionCollection extensions) { - return convert( - relRoot, - relOptCluster, - catalogReader, - parserConfig, - extensions, - ImmutableFeatureBoard.builder().build()); + this.expressionRexConverter = converterProvider.getExpressionRexConverter(this); } public static RelNode convert( - Rel relRoot, - RelOptCluster relOptCluster, - Prepare.CatalogReader catalogReader, - SqlParser.Config parserConfig, - SimpleExtension.ExtensionCollection extensions, - FeatureBoard featureBoard) { + Rel relRoot, Prepare.CatalogReader catalogReader, ConverterProvider converterProvider) { RelBuilder relBuilder = RelBuilder.create( Frameworks.newConfigBuilder() - .parserConfig(parserConfig) + .parserConfig(converterProvider.getSqlParserConfig()) .defaultSchema(catalogReader.getRootSchema().plus()) .traitDefs((List) null) .programs() .build()); - return relRoot.accept( - new SubstraitRelNodeConverter( - extensions, relOptCluster.getTypeFactory(), relBuilder, featureBoard), - Context.newContext()); + converterProvider.getSubstraitRelNodeConverter(relBuilder), Context.newContext()); } @Override diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 126a284f8..99763513c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -63,11 +63,16 @@ import org.apache.calcite.util.ImmutableBitSet; import org.immutables.value.Value; +/** + * SubstraitRelVisitor is used to convert Calcite {@link RelNode}s to Substrait {@link Rel}s. + * + *

Conversion behaviours can be customized by using a {@link ConverterProvider} and/or extending + * this class + */ @SuppressWarnings("UnstableApiUsage") @Value.Enclosing public class SubstraitRelVisitor extends RelNodeVisitor { - private static final FeatureBoard FEATURES_DEFAULT = ImmutableFeatureBoard.builder().build(); private static final Expression.BoolLiteral TRUE = ExpressionCreator.bool(false, true); protected final RexExpressionConverter rexExpressionConverter; @@ -75,15 +80,10 @@ public class SubstraitRelVisitor extends RelNodeVisitor { protected final TypeConverter typeConverter; private Map fieldAccessDepthMap; + /** Use {@link SubstraitRelVisitor#SubstraitRelVisitor(ConverterProvider)} */ + @Deprecated public SubstraitRelVisitor( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { - this(typeFactory, extensions, FEATURES_DEFAULT); - } - - public SubstraitRelVisitor( - RelDataTypeFactory typeFactory, - SimpleExtension.ExtensionCollection extensions, - FeatureBoard features) { this(new ConverterProvider(typeFactory, extensions)); } @@ -94,8 +94,7 @@ public SubstraitRelVisitor( ScalarFunctionConverter scalarFunctionConverter, AggregateFunctionConverter aggregateFunctionConverter, WindowFunctionConverter windowFunctionConverter, - TypeConverter typeConverter, - FeatureBoard features) { + TypeConverter typeConverter) { this( new ConverterProvider( typeFactory, @@ -106,12 +105,9 @@ public SubstraitRelVisitor( } public SubstraitRelVisitor(ConverterProvider converterProvider) { - List converters = converterProvider.getCallConverters(); this.typeConverter = converterProvider.getTypeConverter(); this.aggregateFunctionConverter = converterProvider.getAggregateFunctionConverter(); - this.rexExpressionConverter = - new RexExpressionConverter( - this, converters, converterProvider.getWindowFunctionConverter(), typeConverter); + this.rexExpressionConverter = converterProvider.getRexExpressionConverter(this); } protected Expression toExpression(RexNode node) { @@ -602,39 +598,32 @@ public List apply(List inputs) { } /** - * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using default features. - * - *

This is a convenience method that delegates to {@link #convert(RelRoot, - * SimpleExtension.ExtensionCollection, FeatureBoard)} using {@link #FEATURES_DEFAULT}. + * Deprecated, use {@link #convert(RelRoot, ConverterProvider)} directly * * @param relRoot The Calcite RelRoot to convert. * @param extensions The extension collection to use for the conversion. * @return The resulting Substrait Plan.Root. */ + @Deprecated public static Plan.Root convert(RelRoot relRoot, SimpleExtension.ExtensionCollection extensions) { - return convert(relRoot, extensions, FEATURES_DEFAULT); + return convert(relRoot, new ConverterProvider(extensions)); } /** - * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using a custom visitor. - * - *

This is the main conversion entry point for a complete plan. It applies the provided {@link - * SubstraitRelVisitor} to the final projected {@link RelNode} from the {@code relRoot}, and wraps - * the resulting {@link Rel} in a {@link Plan.Root}. + * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} * - *

This method also correctly extracts the final output field names, paying special attention - * to nested types (structs, maps) via the visitor's type converter, rather than using the names - * from {@code relRoot.validatedRowType} directly. + *

Converts the output of {@link RelRoot#project()} to a Substrait {@link Rel} and wraps it in + * a {@link Plan.Root}. Handles the extraction of final output field names, paying special + * attention to nested types (structs, maps) via the visitor's type converter, rather than using + * the names from {@link RelRoot#validatedRowType} directly. * - * @param relRoot The Calcite RelRoot to convert. This is expected to be a complete, optimized - * plan. - * @param visitor {@link SubstraitRelVisitor} or its subclass. This allows for custom visitor - * behavior. - * @return The resulting Substrait Plan.Root, containing the converted relational tree and the - * output names. + * @param relRoot The Calcite RelRoot to convert. This is expected to be a complete plan. + * @param converterProvider The {@link ConverterProvider} controlling conversion behaviours. + * @return The resulting Substrait {@link Plan.Root}, containing the converted relational tree and + * the output names. */ public static Plan.Root convert(RelRoot relRoot, ConverterProvider converterProvider) { - SubstraitRelVisitor visitor = new SubstraitRelVisitor(converterProvider); + SubstraitRelVisitor visitor = converterProvider.getSubstraitRelVisitor(); visitor.popFieldAccessDepthMap(relRoot.rel); Rel rel = visitor.apply(relRoot.project()); @@ -645,108 +634,31 @@ public static Plan.Root convert(RelRoot relRoot, ConverterProvider converterProv } /** - * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using a custom visitor. - * - *

This is the main conversion entry point for a complete plan. It applies the provided {@link - * SubstraitRelVisitor} to the final projected {@link RelNode} from the {@code relRoot}, and wraps - * the resulting {@link Rel} in a {@link Plan.Root}. - * - *

This method also correctly extracts the final output field names, paying special attention - * to nested types (structs, maps) via the visitor's type converter, rather than using the names - * from {@code relRoot.validatedRowType} directly. - * - * @param relRoot The Calcite RelRoot to convert. This is expected to be a complete, optimized - * plan. - * @param visitor {@link SubstraitRelVisitor} or its subclass. This allows for custom visitor - * behavior. - * @return The resulting Substrait Plan.Root, containing the converted relational tree and the - * output names. - */ - public static Plan.Root convert(RelRoot relRoot, SubstraitRelVisitor visitor) { - visitor.popFieldAccessDepthMap(relRoot.rel); - Rel rel = visitor.apply(relRoot.project()); - - // Avoid using the names from relRoot.validatedRowType because if there are - // nested types (i.e ROW, MAP, etc) the typeConverter will pad names correctly - List names = visitor.typeConverter.toNamedStruct(relRoot.validatedRowType).names(); - return Plan.Root.builder().input(rel).names(names).build(); - } - - /** - * Converts a Calcite {@link RelRoot} to a Substrait {@link Plan.Root} using the specified - * features. - * - *

This is a convenience method that delegates to {@link #convert(RelRoot, - * SubstraitRelVisitor)} using an instance of the {@link SubstraitRelVisitor} as the visitor. - * - * @param relRoot The Calcite RelRoot to convert. - * @param extensions The extension collection to use for the conversion. - * @param features The feature board specifying enabled Substrait features. - * @return The resulting Substrait Plan.Root. - */ - public static Plan.Root convert( - RelRoot relRoot, SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { - return convert( - relRoot, - new SubstraitRelVisitor(relRoot.rel.getCluster().getTypeFactory(), extensions, features)); - } - - /** - * Converts a Calcite {@link RelNode} to a Substrait {@link Rel} using default features. + * Deprecated, use {@link #convert(RelNode, ConverterProvider)} directly * *

This method is suitable for converting a relational sub-tree, but it does not produce a * {@link Plan.Root}. For a complete plan conversion, use {@link #convert(RelRoot, * SimpleExtension.ExtensionCollection)}. * - *

This is a convenience method that delegates to {@link #convert(RelNode, - * SimpleExtension.ExtensionCollection, FeatureBoard)} using {@link #FEATURES_DEFAULT}. - * * @param relNode The Calcite RelNode (and its subtree) to convert. * @param extensions The extension collection to use for the conversion. * @return The resulting Substrait Rel. */ + @Deprecated public static Rel convert(RelNode relNode, SimpleExtension.ExtensionCollection extensions) { - return convert(relNode, extensions, FEATURES_DEFAULT); + return convert(relNode, new ConverterProvider(extensions)); } /** - * Converts a Calcite {@link RelNode} to a Substrait {@link Rel} using a custom visitor. + * Converts a Calcite {@link RelNode} to a Substrait {@link Rel} * - *

This is the main conversion entry point for a partial plan or a single node (and its - * children). It applies the provided {@link SubstraitRelVisitor} to the given {@code relNode}. - * - *

This method does not wrap the result in a {@link Plan.Root} or extract output names. For - * that, use {@link #convert(RelRoot, SubstraitRelVisitor)}. - * - * @param relNode The Calcite RelNode (and its subtree) to convert. - * @param visitor {@link SubstraitRelVisitor} or its subclass. This allows for custom visitor - * behavior. + * @param relNode The Calcite RelNode to convert. + * @param converterProvider The {@link ConverterProvider} controlling conversion behaviours. * @return The resulting Substrait Rel. */ - public static Rel convert(RelNode relNode, SubstraitRelVisitor visitor) { + public static Rel convert(RelNode relNode, ConverterProvider converterProvider) { + SubstraitRelVisitor visitor = converterProvider.getSubstraitRelVisitor(); visitor.popFieldAccessDepthMap(relNode); return visitor.apply(relNode); } - - /** - * Converts a Calcite {@link RelNode} to a Substrait {@link Rel} using the specified features. - * - *

This method is suitable for converting a relational sub-tree, but it does not produce a - * {@link Plan.Root}. For a complete plan conversion, use {@link #convert(RelRoot, - * SimpleExtension.ExtensionCollection, FeatureBoard)}. - * - *

This is a convenience method that delegates to {@link #convert(RelNode, - * SubstraitRelVisitor)} using an instance of the {@link SubstraitRelVisitor} as the visitor. - * - * @param relNode The Calcite RelNode (and its subtree) to convert. - * @param extensions The extension collection to use for the conversion. - * @param features The feature board specifying enabled Substrait features. - * @return The resulting Substrait Rel. - */ - public static Rel convert( - RelNode relNode, SimpleExtension.ExtensionCollection extensions, FeatureBoard features) { - return convert( - relNode, - new SubstraitRelVisitor(relNode.getCluster().getTypeFactory(), extensions, features)); - } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java index 60a0e5e39..9418ad6bd 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToCalcite.java @@ -1,6 +1,5 @@ package io.substrait.isthmus; -import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.SubstraitRelNodeConverter.Context; import io.substrait.plan.Plan; import io.substrait.relation.Rel; @@ -16,15 +15,13 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeField; import org.apache.calcite.sql.SqlKind; -import org.apache.calcite.tools.Frameworks; import org.apache.calcite.tools.RelBuilder; import org.apache.calcite.util.Pair; /** * Converts between Substrait {@link Rel}s and Calcite {@link RelNode}s. * - *

Can be extended to customize the {@link RelBuilder} and {@link SubstraitRelNodeConverter} used - * in the conversion. + *

Conversion behaviours can be customized using a {@link ConverterProvider} */ public class SubstraitToCalcite { @@ -32,47 +29,6 @@ public class SubstraitToCalcite { protected final Prepare.CatalogReader catalogReader; protected ConverterProvider converterProvider; - public SubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - this(extensions, typeFactory, TypeConverter.DEFAULT, null); - } - - public SubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - Prepare.CatalogReader catalogReader) { - this(extensions, typeFactory, TypeConverter.DEFAULT, catalogReader); - } - - public SubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - TypeConverter typeConverter) { - this(extensions, typeFactory, typeConverter, null); - } - - public SubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - TypeConverter typeConverter, - Prepare.CatalogReader catalogReader) { - this( - extensions, - typeFactory, - typeConverter, - catalogReader, - ImmutableFeatureBoard.builder().build()); - } - - public SubstraitToCalcite( - SimpleExtension.ExtensionCollection extensions, - RelDataTypeFactory typeFactory, - TypeConverter typeConverter, - Prepare.CatalogReader catalogReader, - FeatureBoard featureBoard) { - this(new ConverterProvider(typeFactory, extensions), catalogReader); - } - public SubstraitToCalcite(ConverterProvider converterProvider) { this(converterProvider, null); } @@ -84,42 +40,19 @@ public SubstraitToCalcite( this.catalogReader = catalogReader; } - /** - * Extracts a {@link CalciteSchema} from a {@link Rel} - * - *

Override this method to customize schema extraction. - */ - protected CalciteSchema toSchema(Rel rel) { - SchemaCollector schemaCollector = new SchemaCollector(converterProvider); - return schemaCollector.toSchema(rel); - } - - /** - * Creates a {@link RelBuilder} from the extracted {@link CalciteSchema} - * - *

Override this method to customize the {@link RelBuilder}. - */ - protected RelBuilder createRelBuilder(CalciteSchema schema) { - return RelBuilder.create(Frameworks.newConfigBuilder().defaultSchema(schema.plus()).build()); - } - /** * Converts a Substrait {@link Rel} to a Calcite {@link RelNode} * - *

Generates a {@link CalciteSchema} based on the contents of the {@link Rel}, which will be - * used to construct a {@link RelBuilder} with the required schema information to build {@link - * RelNode}s, and a then a {@link SubstraitRelNodeConverter} to perform the actual conversion. - * * @param rel {@link Rel} to convert * @return {@link RelNode} */ public RelNode convert(Rel rel) { RelBuilder relBuilder; if (catalogReader != null) { - relBuilder = createRelBuilder(catalogReader.getRootSchema()); + relBuilder = converterProvider.getRelBuilder(catalogReader.getRootSchema()); } else { - CalciteSchema rootSchema = toSchema(rel); - relBuilder = createRelBuilder(rootSchema); + CalciteSchema rootSchema = converterProvider.getSchemaResolver().apply(rel); + relBuilder = converterProvider.getRelBuilder(rootSchema); } SubstraitRelNodeConverter converter = converterProvider.getSubstraitRelNodeConverter(relBuilder); diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java index db86aeeee..2175fd871 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java @@ -11,23 +11,32 @@ import org.apache.calcite.rel.rel2sql.RelToSqlConverter; import org.apache.calcite.sql.SqlDialect; +/** + * SubstraitToSql assists with converting Substrait to SQL + * + *

Conversion behaviours can be customized using a {@link ConverterProvider} + */ public class SubstraitToSql extends SqlConverterBase { protected SubstraitToCalcite substraitToCalcite; public SubstraitToSql() { - super(FEATURES_DEFAULT); + this(new ConverterProvider()); } + /** Deprecated, use {@link #SubstraitToSql(ConverterProvider)} instead */ + @Deprecated public SubstraitToSql(SimpleExtension.ExtensionCollection extensions) { - super(FEATURES_DEFAULT, extensions); + this(new ConverterProvider(extensions)); + } - substraitToCalcite = new SubstraitToCalcite(extensions, factory); + public SubstraitToSql(ConverterProvider converterProvider) { + super(converterProvider); + substraitToCalcite = converterProvider.getSubstraitToCalcite(); } public RelNode substraitRelToCalciteRel(Rel relRoot, Prepare.CatalogReader catalog) { - return SubstraitRelNodeConverter.convert( - relRoot, relOptCluster, catalog, parserConfig, extensionCollection); + return SubstraitRelNodeConverter.convert(relRoot, catalog, converterProvider); } /** From 74cdf175b864e55ff3ebe671dbf8f1bccd51cb05 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Tue, 23 Dec 2025 11:17:02 -0800 Subject: [PATCH 16/28] refactor: remove FeatureBoard --- .../io/substrait/isthmus/FeatureBoard.java | 20 ------------------- 1 file changed, 20 deletions(-) delete mode 100644 isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java b/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java deleted file mode 100644 index 8db29f73c..000000000 --- a/isthmus/src/main/java/io/substrait/isthmus/FeatureBoard.java +++ /dev/null @@ -1,20 +0,0 @@ -package io.substrait.isthmus; - -import org.apache.calcite.avatica.util.Casing; -import org.immutables.value.Value; - -/** - * A feature board is a collection of flags that are enabled or configurations that control the - * handling of a request to convert query [batch] to Substrait plans. - */ -@Value.Immutable -public abstract class FeatureBoard { - - /** - * @return Calcite's identifier casing policy for unquoted identifiers. - */ - @Value.Default - public Casing unquotedCasing() { - return Casing.TO_UPPER; - } -} From f1e4107b70562d92269522aac7904cb233187ae8 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Tue, 23 Dec 2025 14:45:19 -0800 Subject: [PATCH 17/28] feat: expose ConverterProvider methods --- .../substrait/isthmus/ConverterProvider.java | 22 +++++++++---------- .../isthmus/DynamicConverterProvider.java | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 5734a4b68..f51339917 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -91,7 +91,7 @@ public ConverterProvider( * {@link SqlParser.Config} is a Calcite class which controls SQL parsing behaviour like * identifier casing. */ - protected SqlParser.Config getSqlParserConfig() { + public SqlParser.Config getSqlParserConfig() { return SqlParser.Config.DEFAULT .withUnquotedCasing(Casing.TO_UPPER) .withParserFactory(SqlDdlParserImpl.FACTORY) @@ -102,7 +102,7 @@ protected SqlParser.Config getSqlParserConfig() { * {@link CalciteConnectionConfig} is a Calcite class which controls SQL processing behaviour like * table name case-sensitivity. */ - protected CalciteConnectionConfig getCalciteConnectionConfig() { + public CalciteConnectionConfig getCalciteConnectionConfig() { return CalciteConnectionConfig.DEFAULT.set(CalciteConnectionProperty.CASE_SENSITIVE, "false"); } @@ -110,7 +110,7 @@ protected CalciteConnectionConfig getCalciteConnectionConfig() { * {@link SqlToRelConverter.Config} is a Calcite class which controls SQL processing behaviour * like field-trimming. */ - protected SqlToRelConverter.Config getSqlToRelConverterConfig() { + public SqlToRelConverter.Config getSqlToRelConverterConfig() { return SqlToRelConverter.config().withTrimUnusedFields(true).withExpand(false); } @@ -119,7 +119,7 @@ protected SqlToRelConverter.Config getSqlToRelConverterConfig() { * org.apache.calcite.sql.SqlOperator}s available and controls valid identifiers during SQL * processing. */ - protected SqlOperatorTable getSqlOperatorTable() { + public SqlOperatorTable getSqlOperatorTable() { return SubstraitOperatorTable.INSTANCE; } @@ -152,7 +152,7 @@ protected SubstraitToCalcite getSubstraitToCalcite(Prepare.CatalogReader catalog * A {@link SubstraitRelVisitor} converts Calcite {@link org.apache.calcite.rel.RelNode}s to * Substrait {@link Rel}s */ - protected SubstraitRelVisitor getSubstraitRelVisitor() { + public SubstraitRelVisitor getSubstraitRelVisitor() { return new SubstraitRelVisitor(this); } @@ -160,7 +160,7 @@ protected SubstraitRelVisitor getSubstraitRelVisitor() { * A {@link RexExpressionConverter} converts Calcite {@link org.apache.calcite.rex.RexNode}s to * Substrait equivalents. */ - protected RexExpressionConverter getRexExpressionConverter(SubstraitRelVisitor srv) { + public RexExpressionConverter getRexExpressionConverter(SubstraitRelVisitor srv) { return new RexExpressionConverter( srv, getCallConverters(), getWindowFunctionConverter(), getTypeConverter()); } @@ -169,7 +169,7 @@ protected RexExpressionConverter getRexExpressionConverter(SubstraitRelVisitor s * {@link CallConverter}s are used to convert Calcite {@link org.apache.calcite.rex.RexCall}s to * Substrait equivalents. */ - protected List getCallConverters() { + public List getCallConverters() { ArrayList callConverters = new ArrayList<>(); callConverters.add(new FieldSelectionConverter(typeConverter)); callConverters.add(CallConverters.CASE); @@ -191,7 +191,7 @@ protected List getCallConverters() { * *

Override to customize the schema generation behaviour */ - protected Function getSchemaResolver() { + public Function getSchemaResolver() { SchemaCollector schemaCollector = new SchemaCollector(this); return schemaCollector::toSchema; } @@ -200,7 +200,7 @@ protected Function getSchemaResolver() { * A {@link SubstraitRelNodeConverter} is used when converting from Substrait {@link Rel}s to * Calcite {@link org.apache.calcite.rel.RelNode}s. */ - protected SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relBuilder) { + public SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relBuilder) { return new SubstraitRelNodeConverter(relBuilder, this); } @@ -208,7 +208,7 @@ protected SubstraitRelNodeConverter getSubstraitRelNodeConverter(RelBuilder relB * A {@link ExpressionRexConverter} converts Substrait {@link io.substrait.expression.Expression} * to Calcite equivalents */ - protected ExpressionRexConverter getExpressionRexConverter( + public ExpressionRexConverter getExpressionRexConverter( SubstraitRelNodeConverter relNodeConverter) { ExpressionRexConverter erc = new ExpressionRexConverter( @@ -224,7 +224,7 @@ protected ExpressionRexConverter getExpressionRexConverter( * A {@link RelBuilder} is a Calcite class used as a factory for creating {@link * org.apache.calcite.rel.RelNode}s. */ - protected RelBuilder getRelBuilder(CalciteSchema schema) { + public RelBuilder getRelBuilder(CalciteSchema schema) { return RelBuilder.create(Frameworks.newConfigBuilder().defaultSchema(schema.plus()).build()); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 05b6e891c..297220deb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -24,7 +24,7 @@ public DynamicConverterProvider( } @Override - protected List getCallConverters() { + public List getCallConverters() { List callConverters = super.getCallConverters(); SimpleExtension.ExtensionCollection dynamicExtensionCollection = @@ -45,7 +45,7 @@ protected List getCallConverters() { } @Override - protected SqlOperatorTable getSqlOperatorTable() { + public SqlOperatorTable getSqlOperatorTable() { SimpleExtension.ExtensionCollection dynamicExtensionCollection = ExtensionUtils.getDynamicExtensions(extensions); if (!dynamicExtensionCollection.scalarFunctions().isEmpty() From 686eca76be5e39fcfe68f099294894fdd1cf5430 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 15:38:30 -0800 Subject: [PATCH 18/28] refactor: remove new but deprecated constructors --- .../io/substrait/isthmus/SqlToSubstrait.java | 10 +--------- .../isthmus/SubstraitRelVisitor.java | 19 ------------------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index 0a61986f9..75747247b 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -1,7 +1,5 @@ package io.substrait.isthmus; -import io.substrait.extension.DefaultExtensionCatalog; -import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.sql.SubstraitSqlToCalcite; import io.substrait.plan.ImmutablePlan.Builder; import io.substrait.plan.Plan; @@ -22,13 +20,7 @@ public class SqlToSubstrait extends SqlConverterBase { protected final ConverterProvider converterProvider; public SqlToSubstrait() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION); - } - - /** Use {@link SqlToSubstrait#SqlToSubstrait(ConverterProvider)} instead */ - @Deprecated - public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions) { - this(new ConverterProvider(extensions)); + this(new ConverterProvider()); } public SqlToSubstrait(ConverterProvider converterProvider) { diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 99763513c..e7135c6c6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -10,8 +10,6 @@ import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.LiteralConverter; import io.substrait.isthmus.expression.RexExpressionConverter; -import io.substrait.isthmus.expression.ScalarFunctionConverter; -import io.substrait.isthmus.expression.WindowFunctionConverter; import io.substrait.plan.Plan; import io.substrait.relation.AbstractDdlRel; import io.substrait.relation.AbstractWriteRel; @@ -87,23 +85,6 @@ public SubstraitRelVisitor( this(new ConverterProvider(typeFactory, extensions)); } - /** Use {@link SubstraitRelVisitor#SubstraitRelVisitor(ConverterProvider)} */ - @Deprecated - public SubstraitRelVisitor( - RelDataTypeFactory typeFactory, - ScalarFunctionConverter scalarFunctionConverter, - AggregateFunctionConverter aggregateFunctionConverter, - WindowFunctionConverter windowFunctionConverter, - TypeConverter typeConverter) { - this( - new ConverterProvider( - typeFactory, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, - typeConverter)); - } - public SubstraitRelVisitor(ConverterProvider converterProvider) { this.typeConverter = converterProvider.getTypeConverter(); this.aggregateFunctionConverter = converterProvider.getAggregateFunctionConverter(); From 19833b604f9273bd8ce9c05101f5dd2964c7161d Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 15:46:38 -0800 Subject: [PATCH 19/28] refactor: simplify DynamicConverterProvider#getSqlOperatorTable --- .../isthmus/DynamicConverterProvider.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 297220deb..aa3b9a4f0 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -1,7 +1,6 @@ package io.substrait.isthmus; import io.substrait.extension.SimpleExtension; -import io.substrait.isthmus.calcite.SubstraitOperatorTable; import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.ScalarFunctionConverter; import java.util.Collections; @@ -46,17 +45,16 @@ public List getCallConverters() { @Override public SqlOperatorTable getSqlOperatorTable() { + SqlOperatorTable operatorTable = super.getSqlOperatorTable(); SimpleExtension.ExtensionCollection dynamicExtensionCollection = ExtensionUtils.getDynamicExtensions(extensions); - if (!dynamicExtensionCollection.scalarFunctions().isEmpty() - || !dynamicExtensionCollection.aggregateFunctions().isEmpty()) { - List generatedDynamicOperators = - SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); - return SqlOperatorTables.chain( - SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedDynamicOperators)); + if (dynamicExtensionCollection.scalarFunctions().isEmpty() + && dynamicExtensionCollection.aggregateFunctions().isEmpty()) { + return operatorTable; } - - return SubstraitOperatorTable.INSTANCE; + List generatedDynamicOperators = + SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, typeFactory); + return SqlOperatorTables.chain(operatorTable, SqlOperatorTables.of(generatedDynamicOperators)); } @Override From 724a1e30ecd64e0cf087c64e133560c95082135a Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 16:06:08 -0800 Subject: [PATCH 20/28] refactor: self-review cleanups --- .../main/java/io/substrait/isthmus/SchemaCollector.java | 1 + .../io/substrait/isthmus/SqlExpressionToSubstrait.java | 9 +-------- .../main/java/io/substrait/isthmus/SqlToSubstrait.java | 2 -- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java index 45581b2fb..09a3ebee9 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java @@ -25,6 +25,7 @@ public class SchemaCollector { private final RelDataTypeFactory typeFactory; private final TypeConverter typeConverter; + @Deprecated public SchemaCollector(RelDataTypeFactory typeFactory, TypeConverter typeConverter) { this.typeFactory = typeFactory; this.typeConverter = typeConverter; diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java index 3bdc2e56c..b43b3753f 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java @@ -3,8 +3,6 @@ import io.substrait.extendedexpression.ExtendedExpression; import io.substrait.extendedexpression.ExtendedExpressionProtoConverter; import io.substrait.extendedexpression.ImmutableExtendedExpression.Builder; -import io.substrait.extension.DefaultExtensionCatalog; -import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.calcite.SubstraitTable; import io.substrait.isthmus.expression.RexExpressionConverter; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -34,12 +32,7 @@ public class SqlExpressionToSubstrait extends SqlConverterBase { protected final RexExpressionConverter rexConverter; public SqlExpressionToSubstrait() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION); - } - - /** Use {@link #SqlExpressionToSubstrait(ConverterProvider)} instead */ - public SqlExpressionToSubstrait(SimpleExtension.ExtensionCollection extensions) { - this(new ConverterProvider(extensions)); + this(new ConverterProvider()); } public SqlExpressionToSubstrait(ConverterProvider converterProvider) { diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index 75747247b..fd1d8646f 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -17,7 +17,6 @@ */ public class SqlToSubstrait extends SqlConverterBase { private final SqlOperatorTable operatorTable; - protected final ConverterProvider converterProvider; public SqlToSubstrait() { this(new ConverterProvider()); @@ -26,7 +25,6 @@ public SqlToSubstrait() { public SqlToSubstrait(ConverterProvider converterProvider) { super(converterProvider); this.operatorTable = converterProvider.getSqlOperatorTable(); - this.converterProvider = converterProvider; } /** From ea35c47c252a47da670f880e8fe5ee95bff4b665 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 16:15:11 -0800 Subject: [PATCH 21/28] feat: access extension from ConverterProvider --- .../substrait/isthmus/ConverterProvider.java | 15 +++- .../isthmus/DynamicConverterProvider.java | 4 +- .../isthmus/SubstraitRelNodeConverter.java | 2 +- .../isthmus/SubstraitRelVisitor.java | 2 +- .../substrait/isthmus/CustomFunctionTest.java | 82 +++++++++++-------- .../io/substrait/isthmus/PlanTestBase.java | 14 +--- .../isthmus/RelExtensionRoundtripTest.java | 2 +- .../isthmus/UdfSqlSubstraitTest.java | 5 +- 8 files changed, 71 insertions(+), 55 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index f51339917..ce55dbd3a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -48,6 +48,8 @@ public class ConverterProvider { protected RelDataTypeFactory typeFactory; + protected final SimpleExtension.ExtensionCollection extensions; + protected ScalarFunctionConverter scalarFunctionConverter; protected AggregateFunctionConverter aggregateFunctionConverter; protected WindowFunctionConverter windowFunctionConverter; @@ -55,17 +57,18 @@ public class ConverterProvider { protected TypeConverter typeConverter; public ConverterProvider() { - this(SubstraitTypeSystem.TYPE_FACTORY, DefaultExtensionCatalog.DEFAULT_COLLECTION); + this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); } public ConverterProvider(SimpleExtension.ExtensionCollection extensions) { - this(SubstraitTypeSystem.TYPE_FACTORY, extensions); + this(extensions, SubstraitTypeSystem.TYPE_FACTORY); } public ConverterProvider( - RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { + SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { this( typeFactory, + extensions, new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory), new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), @@ -74,11 +77,13 @@ public ConverterProvider( public ConverterProvider( RelDataTypeFactory typeFactory, + SimpleExtension.ExtensionCollection extensions, ScalarFunctionConverter sfc, AggregateFunctionConverter afc, WindowFunctionConverter wfc, TypeConverter tc) { this.typeFactory = typeFactory; + this.extensions = extensions; this.scalarFunctionConverter = sfc; this.aggregateFunctionConverter = afc; this.windowFunctionConverter = wfc; @@ -234,6 +239,10 @@ public RelDataTypeFactory getTypeFactory() { return typeFactory; } + public SimpleExtension.ExtensionCollection getExtensions() { + return extensions; + } + public ScalarFunctionConverter getScalarFunctionConverter() { return scalarFunctionConverter; } diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index aa3b9a4f0..0b07d36b3 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -16,8 +16,8 @@ public class DynamicConverterProvider extends ConverterProvider { private final SimpleExtension.ExtensionCollection extensions; public DynamicConverterProvider( - RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { - super(typeFactory, extensions); + SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { + super(extensions, typeFactory); this.extensions = extensions; this.scalarFunctionConverter = createScalarFunctionConverter(extensions, typeFactory); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 0920cabd3..f07a87a78 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -106,7 +106,7 @@ public SubstraitRelNodeConverter( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory, RelBuilder relBuilder) { - this(relBuilder, new ConverterProvider(typeFactory, extensions)); + this(relBuilder, new ConverterProvider(extensions, typeFactory)); } public SubstraitRelNodeConverter(RelBuilder relBuilder, ConverterProvider converterProvider) { diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index e7135c6c6..6bcc5b01a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -82,7 +82,7 @@ public class SubstraitRelVisitor extends RelNodeVisitor { @Deprecated public SubstraitRelVisitor( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions) { - this(new ConverterProvider(typeFactory, extensions)); + this(new ConverterProvider(extensions, typeFactory)); } public SubstraitRelVisitor(ConverterProvider converterProvider) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java index f445c9338..0d452cedd 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java @@ -96,22 +96,7 @@ public RelDataType toCalcite(Type.UserDefined type) { static final RelDataType varcharArrayType = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT).createArrayType(varcharType, -1); - // Define additional mapping signatures for the custom scalar functions - final List additionalScalarSignatures = - List.of( - FunctionMappings.s(customScalarFn), - FunctionMappings.s(customScalarAnyFn), - FunctionMappings.s(customScalarAnyToAnyFn), - FunctionMappings.s(customScalarAny1Any1ToAny1Fn), - FunctionMappings.s(customScalarAny1Any2ToAny2Fn), - FunctionMappings.s(customScalarListAnyFn), - FunctionMappings.s(customScalarListAnyAndAnyFn), - FunctionMappings.s(customScalarListStringFn), - FunctionMappings.s(customScalarListStringAndAnyFn), - FunctionMappings.s(customScalarListStringAndAnyVariadic0Fn), - FunctionMappings.s(customScalarListStringAndAnyVariadic1Fn), - FunctionMappings.s(toBType)); - + // Define additional signatures for the custom scalar functions static final SqlFunction customScalarFn = new SqlFunction( "custom_scalar", @@ -190,6 +175,7 @@ public RelDataType toCalcite(Type.UserDefined type) { null, null, SqlFunctionCategory.USER_DEFINED_FUNCTION); + static final SqlFunction customScalarListStringAndAnyVariadic0Fn = new SqlFunction( "custom_scalar_liststring_anyvariadic0_to_liststring", @@ -198,6 +184,7 @@ public RelDataType toCalcite(Type.UserDefined type) { null, null, SqlFunctionCategory.USER_DEFINED_FUNCTION); + static final SqlFunction customScalarListStringAndAnyVariadic1Fn = new SqlFunction( "custom_scalar_liststring_anyvariadic1_to_liststring", @@ -216,9 +203,22 @@ public RelDataType toCalcite(Type.UserDefined type) { null, SqlFunctionCategory.USER_DEFINED_FUNCTION); - // Define additional mapping signatures for the custom aggregate functions - final List additionalAggregateSignatures = - List.of(FunctionMappings.s(customAggregateFn)); + static final List additionalScalarSignatures = + List.of( + FunctionMappings.s(customScalarFn), + FunctionMappings.s(customScalarAnyFn), + FunctionMappings.s(customScalarAnyToAnyFn), + FunctionMappings.s(customScalarAny1Any1ToAny1Fn), + FunctionMappings.s(customScalarAny1Any2ToAny2Fn), + FunctionMappings.s(customScalarListAnyFn), + FunctionMappings.s(customScalarListAnyAndAnyFn), + FunctionMappings.s(customScalarListStringFn), + FunctionMappings.s(customScalarListStringAndAnyFn), + FunctionMappings.s(customScalarListStringAndAnyVariadic0Fn), + FunctionMappings.s(customScalarListStringAndAnyVariadic1Fn), + FunctionMappings.s(toBType)); + + // Define additional signatures for the custom aggregate functions static final SqlAggFunction customAggregateFn = new SqlAggFunction( @@ -229,38 +229,50 @@ public RelDataType toCalcite(Type.UserDefined type) { null, SqlFunctionCategory.USER_DEFINED_FUNCTION) {}; - TypeConverter typeConverter = new TypeConverter(userTypeMapper); + static final List additionalAggregateSignatures = + List.of(FunctionMappings.s(customAggregateFn)); + + static TypeConverter typeConverter = new TypeConverter(userTypeMapper); // Create Function Converters that can handle the custom functions - ScalarFunctionConverter scalarFunctionConverter = + static ScalarFunctionConverter scalarFunctionConverter = new ScalarFunctionConverter( CUSTOM_EXTENSIONS.scalarFunctions(), additionalScalarSignatures, - typeFactory, + SubstraitTypeSystem.TYPE_FACTORY, typeConverter); - AggregateFunctionConverter aggregateFunctionConverter = + static AggregateFunctionConverter aggregateFunctionConverter = new AggregateFunctionConverter( CUSTOM_EXTENSIONS.aggregateFunctions(), additionalAggregateSignatures, - typeFactory, - typeConverter); - WindowFunctionConverter windowFunctionConverter = - new WindowFunctionConverter(CUSTOM_EXTENSIONS.windowFunctions(), typeFactory); - - ConverterProvider converterProvider = - new ConverterProvider( - typeFactory, - scalarFunctionConverter, - aggregateFunctionConverter, - windowFunctionConverter, + SubstraitTypeSystem.TYPE_FACTORY, typeConverter); + static WindowFunctionConverter windowFunctionConverter = + new WindowFunctionConverter( + CUSTOM_EXTENSIONS.windowFunctions(), SubstraitTypeSystem.TYPE_FACTORY); + + // static ConverterProvider converterProvider = + // new ConverterProvider( + // SubstraitTypeSystem.TYPE_FACTORY, + // CUSTOM_EXTENSIONS, + // scalarFunctionConverter, + // aggregateFunctionConverter, + // windowFunctionConverter, + // typeConverter); // Create a SubstraitRelVisitor that uses the custom Function Converters final SubstraitRelVisitor calciteToSubstrait = new SubstraitRelVisitor(converterProvider); final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider); CustomFunctionTest() { - super(CUSTOM_EXTENSIONS); + super( + new ConverterProvider( + SubstraitTypeSystem.TYPE_FACTORY, + CUSTOM_EXTENSIONS, + scalarFunctionConverter, + aggregateFunctionConverter, + windowFunctionConverter, + typeConverter)); } @Test diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index 49e986c85..9d3c8135a 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -7,7 +7,6 @@ import com.google.common.annotations.Beta; import com.google.common.io.Resources; import io.substrait.dsl.SubstraitBuilder; -import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -71,18 +70,13 @@ public class PlanTestBase { PlanTestBase.schemaToCatalog("tpcds", TPCDS_SCHEMA); protected PlanTestBase() { - this(DefaultExtensionCatalog.DEFAULT_COLLECTION); + this(new ConverterProvider()); } - protected PlanTestBase(SimpleExtension.ExtensionCollection extensions) { - this(extensions, new ConverterProvider(extensions)); - } - - protected PlanTestBase( - SimpleExtension.ExtensionCollection extensions, ConverterProvider converterProvider) { - this.extensions = extensions; - this.sb = new SubstraitBuilder(extensions); + protected PlanTestBase(ConverterProvider converterProvider) { this.converterProvider = converterProvider; + this.extensions = converterProvider.getExtensions(); + this.sb = new SubstraitBuilder(extensions); this.substraitToCalcite = new SubstraitToCalcite(converterProvider); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java index 537cc6988..5ec26b92b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/RelExtensionRoundtripTest.java @@ -82,7 +82,7 @@ void roundtrip(Rel pojo1) { // Calcite -> Substrait POJO 3 Rel pojo3 = - (new CustomSubstraitRelVisitor(new ConverterProvider(typeFactory, extensions))) + (new CustomSubstraitRelVisitor(new ConverterProvider(extensions, typeFactory))) .apply(calcite); assertEquals(pojo1, pojo3); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java index 3bab99bd5..6763b1c4b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java @@ -12,8 +12,9 @@ class UdfSqlSubstraitTest extends PlanTestBase { private static final String CUSTOM_FUNCTION_PATH = "/extensions/scalar_functions_custom.yaml"; UdfSqlSubstraitTest() { - super(loadExtensions(List.of(CUSTOM_FUNCTION_PATH))); - this.converterProvider = new DynamicConverterProvider(typeFactory, extensions); + super( + new DynamicConverterProvider( + loadExtensions(List.of(CUSTOM_FUNCTION_PATH)), SubstraitTypeSystem.TYPE_FACTORY)); } @Test From d050df57c9f7e748bb556dca85073a42d03c394d Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 16:26:29 -0800 Subject: [PATCH 22/28] refactor: missed constructor update in IsthmusEntryPoint --- .../main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index a700cc0ff..ab6b21d9c 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -3,7 +3,6 @@ import com.google.protobuf.Message; import com.google.protobuf.TextFormat; import com.google.protobuf.util.JsonFormat; -import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.isthmus.SqlExpressionToSubstrait; import io.substrait.isthmus.SqlToSubstrait; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -82,8 +81,7 @@ public static void main(String... args) { public Integer call() throws Exception { // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { - SqlExpressionToSubstrait converter = - new SqlExpressionToSubstrait(DefaultExtensionCatalog.DEFAULT_COLLECTION); + SqlExpressionToSubstrait converter = new SqlExpressionToSubstrait(); ExtendedExpression extendedExpression = converter.convert(sqlExpressions, createStatements); printMessage(extendedExpression); } else { // by default Isthmus image are parsing SQL Query From 9fed4cc74d9ce9cd3414da855b908e566c8332d6 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Wed, 18 Feb 2026 16:57:42 -0800 Subject: [PATCH 23/28] refactor: more cleanup --- .../io/substrait/isthmus/DynamicConverterProvider.java | 5 ----- .../java/io/substrait/isthmus/CustomFunctionTest.java | 9 --------- 2 files changed, 14 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 0b07d36b3..7f1cde2c1 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -57,11 +57,6 @@ public SqlOperatorTable getSqlOperatorTable() { return SqlOperatorTables.chain(operatorTable, SqlOperatorTables.of(generatedDynamicOperators)); } - @Override - public ScalarFunctionConverter getScalarFunctionConverter() { - return scalarFunctionConverter; - } - private static ScalarFunctionConverter createScalarFunctionConverter( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java index 0d452cedd..3297e3c3d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java @@ -251,15 +251,6 @@ public RelDataType toCalcite(Type.UserDefined type) { new WindowFunctionConverter( CUSTOM_EXTENSIONS.windowFunctions(), SubstraitTypeSystem.TYPE_FACTORY); - // static ConverterProvider converterProvider = - // new ConverterProvider( - // SubstraitTypeSystem.TYPE_FACTORY, - // CUSTOM_EXTENSIONS, - // scalarFunctionConverter, - // aggregateFunctionConverter, - // windowFunctionConverter, - // typeConverter); - // Create a SubstraitRelVisitor that uses the custom Function Converters final SubstraitRelVisitor calciteToSubstrait = new SubstraitRelVisitor(converterProvider); final SubstraitToCalcite substraitToCalcite = new SubstraitToCalcite(converterProvider); From 7d98badec86e48eb2f931c155af2f65ae02d9bc2 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 19 Feb 2026 17:18:21 -0800 Subject: [PATCH 24/28] feat: additional DynamicConverter constructors --- .../io/substrait/isthmus/DynamicConverterProvider.java | 9 +++++++++ .../java/io/substrait/isthmus/UdfSqlSubstraitTest.java | 4 +--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 7f1cde2c1..295c5818d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -1,5 +1,6 @@ package io.substrait.isthmus; +import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.ScalarFunctionConverter; @@ -15,6 +16,14 @@ public class DynamicConverterProvider extends ConverterProvider { private final SimpleExtension.ExtensionCollection extensions; + public DynamicConverterProvider() { + this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); + } + + public DynamicConverterProvider(SimpleExtension.ExtensionCollection extensions) { + this(extensions, SubstraitTypeSystem.TYPE_FACTORY); + } + public DynamicConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { super(extensions, typeFactory); diff --git a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java index 6763b1c4b..cbdbe2fa3 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/UdfSqlSubstraitTest.java @@ -12,9 +12,7 @@ class UdfSqlSubstraitTest extends PlanTestBase { private static final String CUSTOM_FUNCTION_PATH = "/extensions/scalar_functions_custom.yaml"; UdfSqlSubstraitTest() { - super( - new DynamicConverterProvider( - loadExtensions(List.of(CUSTOM_FUNCTION_PATH)), SubstraitTypeSystem.TYPE_FACTORY)); + super(new DynamicConverterProvider(loadExtensions(List.of(CUSTOM_FUNCTION_PATH)))); } @Test From d4ade50db5d95e1f9e337ad97a727d2913c304d7 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Thu, 19 Feb 2026 17:27:33 -0800 Subject: [PATCH 25/28] refactor: simplify guard condition --- .../java/io/substrait/isthmus/DynamicConverterProvider.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 295c5818d..345ec2aec 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -69,7 +69,7 @@ public SqlOperatorTable getSqlOperatorTable() { private static ScalarFunctionConverter createScalarFunctionConverter( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - List additionalSignatures; + List additionalSignatures = Collections.emptyList(); java.util.Set knownFunctionNames = FunctionMappings.SCALAR_SIGS.stream() @@ -81,9 +81,7 @@ private static ScalarFunctionConverter createScalarFunctionConverter( .filter(f -> !knownFunctionNames.contains(f.name().toLowerCase())) .collect(Collectors.toList()); - if (dynamicFunctions.isEmpty()) { - additionalSignatures = Collections.emptyList(); - } else { + if (!dynamicFunctions.isEmpty()) { SimpleExtension.ExtensionCollection dynamicExtensionCollection = SimpleExtension.ExtensionCollection.builder().scalarFunctions(dynamicFunctions).build(); From 58b65a26e5c3e22599eb0577d02b93b152eaea9e Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Fri, 20 Feb 2026 09:04:05 -0800 Subject: [PATCH 26/28] feat: handle --unquotedcasing option with ConverterProvider --- .../isthmus/cli/IsthmusEntryPoint.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index ab6b21d9c..03d4fef2b 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -3,6 +3,7 @@ import com.google.protobuf.Message; import com.google.protobuf.TextFormat; import com.google.protobuf.util.JsonFormat; +import io.substrait.isthmus.ConverterProvider; import io.substrait.isthmus.SqlExpressionToSubstrait; import io.substrait.isthmus.SqlToSubstrait; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -12,7 +13,9 @@ import java.io.IOException; import java.util.List; import java.util.concurrent.Callable; +import org.apache.calcite.avatica.util.Casing; import org.apache.calcite.prepare.Prepare; +import org.apache.calcite.sql.parser.SqlParser; import picocli.CommandLine; import picocli.CommandLine.Command; import picocli.CommandLine.Option; @@ -52,6 +55,20 @@ enum OutputFormat { BINARY, // protobuf BINARY format } + @Option( + names = {"--unquotedcasing"}, + description = "Calcite's casing policy for unquoted identifiers: ${COMPLETION-CANDIDATES}") + private Casing unquotedCasing = Casing.TO_UPPER; + + private ConverterProvider converterProvider() { + return new ConverterProvider() { + @Override + public SqlParser.Config getSqlParserConfig() { + return super.getSqlParserConfig().withUnquotedCasing(unquotedCasing); + } + }; + } + /** * Standard Java Main method invoked by the isthmus CLI command. * @@ -81,11 +98,11 @@ public static void main(String... args) { public Integer call() throws Exception { // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { - SqlExpressionToSubstrait converter = new SqlExpressionToSubstrait(); + SqlExpressionToSubstrait converter = new SqlExpressionToSubstrait(converterProvider()); ExtendedExpression extendedExpression = converter.convert(sqlExpressions, createStatements); printMessage(extendedExpression); } else { // by default Isthmus image are parsing SQL Query - SqlToSubstrait converter = new SqlToSubstrait(); + SqlToSubstrait converter = new SqlToSubstrait(converterProvider()); Prepare.CatalogReader catalog = SubstraitCreateStatementParser.processCreateStatementsToCatalog( createStatements.toArray(String[]::new)); From 0b3bbbe12c68c18bc11bd5f3be898f53b1e09a8a Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 23 Feb 2026 10:24:36 -0800 Subject: [PATCH 27/28] refactor: use inherited extension field --- .../java/io/substrait/isthmus/DynamicConverterProvider.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 345ec2aec..4cc2a397e 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -14,8 +14,6 @@ public class DynamicConverterProvider extends ConverterProvider { - private final SimpleExtension.ExtensionCollection extensions; - public DynamicConverterProvider() { this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY); } @@ -27,7 +25,6 @@ public DynamicConverterProvider(SimpleExtension.ExtensionCollection extensions) public DynamicConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { super(extensions, typeFactory); - this.extensions = extensions; this.scalarFunctionConverter = createScalarFunctionConverter(extensions, typeFactory); } From 47e678889247fd7539905bf2fcc41cf63c39fda4 Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Mon, 23 Feb 2026 10:28:34 -0800 Subject: [PATCH 28/28] feat: createScalarFunctionConverter should use ConverterProvider fields --- .../substrait/isthmus/DynamicConverterProvider.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java index 4cc2a397e..0b6462d18 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/DynamicConverterProvider.java @@ -25,7 +25,7 @@ public DynamicConverterProvider(SimpleExtension.ExtensionCollection extensions) public DynamicConverterProvider( SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { super(extensions, typeFactory); - this.scalarFunctionConverter = createScalarFunctionConverter(extensions, typeFactory); + this.scalarFunctionConverter = createScalarFunctionConverter(); } @Override @@ -42,10 +42,7 @@ public List getCallConverters() { .collect(Collectors.toList()); callConverters.add( new ScalarFunctionConverter( - extensions.scalarFunctions(), - additionalSignatures, - typeFactory, - TypeConverter.DEFAULT)); + extensions.scalarFunctions(), additionalSignatures, typeFactory, typeConverter)); return callConverters; } @@ -63,9 +60,7 @@ public SqlOperatorTable getSqlOperatorTable() { return SqlOperatorTables.chain(operatorTable, SqlOperatorTables.of(generatedDynamicOperators)); } - private static ScalarFunctionConverter createScalarFunctionConverter( - SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) { - + private ScalarFunctionConverter createScalarFunctionConverter() { List additionalSignatures = Collections.emptyList(); java.util.Set knownFunctionNames = @@ -92,6 +87,6 @@ private static ScalarFunctionConverter createScalarFunctionConverter( } return new ScalarFunctionConverter( - extensions.scalarFunctions(), additionalSignatures, typeFactory, TypeConverter.DEFAULT); + extensions.scalarFunctions(), additionalSignatures, typeFactory, typeConverter); } }