Add udf support for Substrait<->Calcite conversion #457

ZorinAnton · 2025-08-05T12:58:14Z

This PR introduces support for dynamic scalar User-Defined Functions (UDFs) for round-trip conversion between Substrait plans and Calcite RelNodes.

It establishes the Substrait YAML extension files as the single source of truth (SSoT) for all function signatures. The primary addition is a new utility, YamlToSqlOperator, which acts as a bridge between the Substrait and Calcite worlds by dynamically generating Calcite SqlOperator objects from the parsed YAML definitions.

The implementation is optimized to only generate these operators for functions that are truly dynamic —that is, functions present in the provided extensions but not already defined in the default SubstraitOperatorTable.

Key Changes:

New YamlToSqlOperator Utility: A type-safe converter that translates SimpleExtension.ExtensionCollection objects into a List.
Enhanced SqlToSubstrait (SQL → Substrait): The SqlToSubstrait constructor is updated to:
- Identify dynamic UDFs by filtering the loaded extensions against the known operators in SubstraitOperatorTable.
- Use YamlToSqlOperator to generate operators for these dynamic UDFs.
- Inject the new operators into Calcite's validator via a сhained SqlOperatorTable.
Enhanced SubstraitToCalcite (Substrait → SQL): The SubstraitRelNodeConverter is similarly enhanced to:
- Also identify dynamic UDFs from the provided SimpleExtension.ExtensionCollection.
- Dynamically create FunctionMappings.Sig objects for these UDFs, enabling the ScalarFunctionConverter to correctly map them back to their SqlOperator during plan conversion.
Architectural Refactoring: The EXTENSION_COLLECTION in SqlConverterBase has been changed from static final to an instance field to allow for the dynamic configuration required by this feature.

vbarua

I started taking a look at this today, but didn't have time to finish.

I'm going to restate what I think your doing with these changes to make sure that we're on the same page.

To start, you're generating SqlOperators dynamically from the function definitions in SimpleExtensions. You then use these to generate a SqlOperatorTable that covers functions that aren't already in the SubstraitOperatorTable.

This let's you do things like parse SQL queries with functions that are defined only in the SimpleExtension, and aren't part of the SubstraitOperatorTable.

With these definitions, you can also add additional mappings when converting from Substrait to Calcite, and back, for functions that aren't explicitly mapped in the static SCALAR_SIGS mapping.

Meta Comment

I find the usage of "custom" for this concept somewhat confusing. These aren't really custom functions because they are part of the core Substrait spec. A better word for this might be "dynamic". In my head, you're effectively generating SqlOperators dynamically based on the Substrait definitions, and then using them as fall-backs when explicit mappings.

vbarua · 2025-08-08T00:16:41Z

isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlValidator.java

@@ -12,4 +13,8 @@ public class SubstraitSqlValidator extends SqlValidatorImpl {
  public SubstraitSqlValidator(Prepare.CatalogReader catalogReader) {
    super(SubstraitOperatorTable.INSTANCE, catalogReader, catalogReader.getTypeFactory(), CONFIG);
  }
+
+  public SubstraitSqlValidator(SqlOperatorTable opTable, Prepare.CatalogReader catalogReader) {


minor: It's slightly nicer API-wise if we re-order the arguments so that the constructors are:

public SubstraitSqlValidator(Prepare.CatalogReader catalogReader) public SubstraitSqlValidator(Prepare.CatalogReader catalogReader, SqlOperatorTable opTable)

vbarua · 2025-08-08T00:20:52Z

isthmus/src/main/java/io/substrait/isthmus/YamlToSqlOperator.java

We should call this SimpleExtensionToSqlOperator to more accurately capture what the conversion is.

vbarua · 2025-08-08T00:28:59Z

isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java

@@ -10,7 +12,21 @@ public SubstraitToSql() {
    super(FEATURES_DEFAULT);
  }

+  public SubstraitToSql(List<String> yamlFunctionFiles) {


I think this would be better to keep this API simple and just make it:

SubstraitToSql(SimpleExtension.ExtensionCollection extensions)

Users can then handle loading and merging extensions any way they see fit.

vbarua · 2025-08-08T00:42:43Z

isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java

+    SimpleExtension.ExtensionCollection customExtensionCollection =
+        ExtensionUtils.getCustomExtensions(extensions);
+    List<SqlOperator> generatedCustomOperators =
+        YamlToSqlOperator.from(customExtensionCollection, this.factory);


Do we actually need to distinguish between the SqlOperators defined explicitly in the SubstraitOperatorTable and those that you are generating here?

I think we can just generate mappings for every function found in the extension collection, and then chain the operator tables like:

SqlOperatorTables.chain( SubstraitOperatorTable.INSTANCE, SqlOperatorTables.of(generatedCustomOperators) );

we automatically give precedence to the non-custom ones.

I'm not sure that precedence should be given the to default (non-custom) operators. What if users want to override them?

If I understood correctly, you are suggesting to generate SqlOperators for all extensions, the default ones and the dynamic:

List<SqlOperator> generatedCustomOperators = SimpleExtensionToSqlOperator.from(dynamicExtensionCollection, this.factory);

I see two issues here:

duplicated work (default operators already exist in SubstraitOperatorTable.INSTANCE

it requires the fully fledged TypeExpressionEvaluator that will probably be needed at some point anyway, but I guess this should be done in a different PR

I'm not sure that precedence should be given the to default (non-custom) operators.

I would argue that it's preferrable to bind to the built-ins when possible, because internally in both the RelBuilder and optimizer rules Calcite looks for specific operator instances when performing optimization (e.g. comparison simplification, aggregate rewrititing, etc). If we use dynamic operators instead of built-in ones, Calcite can no longer perform these optimizations.

Ok, I changed the order so that built-in operators have precedence.

vbarua · 2025-08-08T00:44:14Z

isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java

+    this(SimpleExtension.loadDefaults(), features);
+  }
+
+  public SqlToSubstrait(SimpleExtension.ExtensionCollection extensions, FeatureBoard features) {


I think this should just be

public SqlToSubstrait(SqlOperatorTable operatorTable, FeatureBoard features)

to give users direct control over the operator table they are using.

In this constructor we need to call the constructor of the base class, SqlConverterBase that requires an instance of SimpleExtension.ExtensionCollection as an argument. Otherwise it uses SimpleExtension.loadDefaults() which will cause an inconsistency between the operatorTable and the extensionCollection fields, as the former will contain operators that don't have corresponding extensions in the latter.

ZorinAnton · 2025-08-08T12:31:40Z

I'm going to restate what I think your doing with these changes to make sure that we're on the same page.

Yes, you've correctly summarized the changes.

Meta Comment

I find the usage of "custom" for this concept somewhat confusing. These aren't really custom functions because they are part of the core Substrait spec. A better word for this might be "dynamic". In my head, you're effectively generating SqlOperators dynamically based on the Substrait definitions, and then using them as fall-backs when explicit mappings.

I renamed all "custom" to "dynamic" in the changes, however in my mind User Defined Functions is always something custom :)

vbarua · 2025-08-11T23:23:35Z

isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java

+      TypeConverter typeConverter) {
+    List<SimpleExtension.ValueArgument> requiredArgs =
+        function.args().stream()
+            .filter(SimpleExtension.Argument::required)


I believe this is meaningless because it's always true if you look at the implementations. I'm not even sure what require is used for actually.

The required method is used here:

substrait-java/core/src/main/java/io/substrait/extension/SimpleExtension.java

Line 235 in ec76720

return args().stream().filter(Argument::required).collect(Collectors.toList());

. I would think it is supposed to reflect whether the argument is required or optional. So probably it is not correct to always return true.

vbarua · 2025-08-11T23:30:09Z

isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java

+    List<SimpleExtension.ValueArgument> requiredArgs =
+        function.args().stream()
+            .filter(SimpleExtension.Argument::required)
+            .filter(t -> t instanceof SimpleExtension.ValueArgument)


I'm not sure if it's valid to ignore Enum arguments when generating functions.

For example in extract

impls: - args: - name: component options: [ YEAR, ISO_YEAR, US_YEAR, HOUR, MINUTE, SECOND, MILLISECOND, MICROSECOND, SUBSECOND, PICOSECOND, UNIX_TIME, TIMEZONE_OFFSET ] description: The part of the value to extract. - name: x value: timestamp_tz - name: timezone description: Timezone string from IANA tzdb. value: string return: i64

the enumeration argument for component is part of the function signature. It might make sense to ignore function with enum arguments to start with.

I added the code to handle EnumArgument, however I'm not sure if currently sql with functions using it can be translated to substrait, at least I couldn't manage to do it. Comments in this function indirectly mention that.

vbarua · 2025-08-11T23:32:02Z

isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java

+          TypeExpressionEvaluator.evaluateExpression(
+              returnExpression, function.args(), substraitArgTypes);
+
+      return typeConverter.toCalcite(typeFactory, resolvedSubstraitType);


You're not using the nullability of the function to determine if the output is nullable or not. We should probably account for it.

Added handling of nullability.

vbarua · 2025-08-11T23:34:41Z

isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java

+        SqlKind.OTHER_FUNCTION,
+        returnTypeInference,
+        null,
+        OperandTypes.family(argFamilies),


I think this works fine for MIRROR and DECLARED_OUTPUT nullabilities, but will potentially overmatch on DISCRETE which puts constraints on the nullability of input arguments when matching functions.

Added handling of all three types of nullability.

# Conflicts: # isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java

…ToSqlOperator

ZorinAnton marked this pull request as draft August 5, 2025 13:05

ZorinAnton force-pushed the zor-udf branch 6 times, most recently from 2f8f79a to ad11957 Compare August 7, 2025 10:08

ZorinAnton marked this pull request as ready for review August 7, 2025 11:15

nielspardon self-requested a review August 7, 2025 13:30

vbarua reviewed Aug 8, 2025

View reviewed changes

ZorinAnton force-pushed the zor-udf branch from ad11957 to 2973628 Compare August 8, 2025 12:18

ZorinAnton force-pushed the zor-udf branch 2 times, most recently from 2356755 to b232a95 Compare August 11, 2025 15:07

vbarua reviewed Aug 11, 2025

View reviewed changes

ZorinAnton force-pushed the zor-udf branch 4 times, most recently from af79a55 to f2527e1 Compare August 14, 2025 13:01

ZorinAnton added 2 commits August 15, 2025 12:37

feat(isthmus): udf support for substrait<->calcite

9932b53

# Conflicts: # isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java

chore(isthmus): handle nullability and EnumArgument in SimplExtension…

e7a3d0a

…ToSqlOperator

ZorinAnton force-pushed the zor-udf branch from f2527e1 to e7a3d0a Compare August 15, 2025 10:37

ZorinAnton requested a review from vbarua August 18, 2025 08:28

Add udf support for Substrait<->Calcite conversion #457

Are you sure you want to change the base?

Add udf support for Substrait<->Calcite conversion #457

Uh oh!

Conversation

ZorinAnton commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbarua left a comment

Choose a reason for hiding this comment

Meta Comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZorinAnton Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZorinAnton commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Meta Comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZorinAnton commented Aug 5, 2025 •

edited

Loading

ZorinAnton Aug 8, 2025 •

edited

Loading

ZorinAnton commented Aug 8, 2025 •

edited

Loading