Skip to content

feat(isthmus): configurable fallback to dynamic function mapping#647

Open
ZorinAnton wants to merge 7 commits intosubstrait-io:mainfrom
ZorinAnton:fallback-to-dynamic-function-mappings
Open

feat(isthmus): configurable fallback to dynamic function mapping#647
ZorinAnton wants to merge 7 commits intosubstrait-io:mainfrom
ZorinAnton:fallback-to-dynamic-function-mappings

Conversation

@ZorinAnton
Copy link
Contributor

This PR introduces a new autoFallbackToDynamicFunctionMapping feature flag that enables automatic support for all unmapped extension functions (scalar, aggregate, and window functions) in SQL↔Substrait conversions.

Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on a PR (#649) to try and make wiring in what you're doing here easier. We're starting to run into the limits of the existing design of the function conversion system, and I'm wary of adding more complexity before cleaning it up.

My big issue is with how we're using the feature flags for this, because we're embedding a ton of conditional configuration logic into constructors all over the codebase, which makes it very difficult to reason about an already difficult part of the system. What I would like to do is separate out the configuration from the constructors. Effectively, can we construct all the function converters we need outside of the SubstraitRelNode and SubstraitRelVisitor, and then inject them in.

@ZorinAnton ZorinAnton force-pushed the fallback-to-dynamic-function-mappings branch 3 times, most recently from 9314af1 to 956b447 Compare March 6, 2026 06:59
@ZorinAnton ZorinAnton marked this pull request as ready for review March 9, 2026 11:09
@ZorinAnton ZorinAnton requested a review from vbarua March 9, 2026 11:10
@nielspardon
Copy link
Member

@ZorinAnton can you rebase one more time and resolve the conflicts? thanks

@ZorinAnton ZorinAnton force-pushed the fallback-to-dynamic-function-mappings branch from a5c465f to 66ed907 Compare March 13, 2026 13:27
@vbarua
Copy link
Member

vbarua commented Mar 13, 2026

High-level question

What's the value of having a separate AutomaticDynamicFunctionMappingConverterProvider instead of expanding the DynamicConverterProvider. Put another way, would it make sense to just have a non-dynamic mode (ConverterProvider) and dynamic mode (DynamicConverterProvider)?

@ZorinAnton
Copy link
Contributor Author

While they use similar mechanics under the hood, they serve two distinct use cases:

  • DynamicConverterProvider is specifically for custom UDFs injected at runtime. It explicitly filters out the standard Substrait catalog.
  • AutomaticDynamicFunctionMappingConverterProvider is a fallback for standard Substrait functions (like strftime) that exist in the official YAMLs but haven't been manually added to our static FunctionMappings.java yet.

Keeping them separate allows users to opt into custom UDF support, the standard function fallback, or both independently.

@bestbeforetoday bestbeforetoday self-assigned this Mar 20, 2026
ZorinAnton and others added 7 commits March 20, 2026 18:02
Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
Decompose into separate functions to clarify intent. Reduce cognitive
completixy.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
The initial implementation relied on non-obvious method side-effects and
lazy initialization of member variables based on those side-effects,
which made the logic hard to understand and fragile. This change
decomposes the logic into small functions with no side-effects, and
initializes all member variable state in the constructor. This has the
added benefit of making the class thread safe.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday bestbeforetoday force-pushed the fallback-to-dynamic-function-mappings branch from 0bb65b8 to 2126363 Compare March 20, 2026 18:19
@bestbeforetoday
Copy link
Member

With the current ConverterProvider structure, it is not clear to me how we expect users to opt in to both custom UDF and standard function fallback. Maybe we need some thought on composable ConverterProvider implementations. However, I think any changes to support that should happen in another PR. This one has already been pending for too long.

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZorinAnton I had some concerns about:

  1. The merge commits instead of rebasing of your changes on the latest main branch state, which made the commit history hard to unpick.
  2. Some implementation details: readability, complexity and fragility.

I have cherry-picked your commits on top of the current main branch content to tidy up the commit history. I also added some additional commits, just to tidy up the implementation slightly. These changes address things I would otherwise have raised as review comments, but I have made the changes myself in the interests of getting this PR merged quickly. The logic and behaviour are unchanged. This is pushed to your PR branch so this PR reflects all of these changes.

Your exact PR branch state before the changes I made above is at bestbeforetoday/anton-fallback-to-dynamic-function-mappings.

I am happy with this PR to be merged as-is so am approving. Another pair of eyes — either yours or another maintainer — might be good as a sanity check before merging.

@ZorinAnton
Copy link
Contributor Author

With the current ConverterProvider structure, it is not clear to me how we expect users to opt in to both custom UDF and standard function fallback.

Potentially, the Composite Pattern can be used. This might cause creation of new class for each combination of features, but at least users could opt in excaclty to what they want.

Another pair of eyes — either yours or another maintainer — might be good as a sanity check before merging.

At first glance I don't any issues, I will take a deeper look over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants