fix: prevents exception on construction of FunctionConverter with duplicate functions
#564
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benbellick
commented
Oct 22, 2025
| this.substraitFuncKeyToSqlOperatorMap = ArrayListMultimap.create(); | ||
|
|
||
| ArrayListMultimap<String, F> alm = ArrayListMultimap.<String, F>create(); | ||
| ArrayListMultimap<String, F> nameToFn = ArrayListMultimap.<String, F>create(); |
Member
Author
There was a problem hiding this comment.
This is unrelated to the PR technically, but I just thought that this was a clearer name.
benbellick
force-pushed
the
ben.bellick/562/function-converter-crash
branch
2 times, most recently
from
20db198 to
f68cb82
Compare
nielspardon
reviewed
Oct 22, 2025
isthmus/src/test/java/io/substrait/isthmus/DuplicateFunctionUrnTest.java
Outdated
Show resolved
Hide resolved
This commit introduces a test to capture the bug where name-colliding extension functions with different URNs cause an exception to be thrown on `FunctionConverter` construction.
This is a small change to make the meaning of a variable clearer in the `FunctionConverter.java` implementation.
benbellick
force-pushed
the
ben.bellick/562/function-converter-crash
branch
from
f68cb82 to
03b0f24
Compare
Added a test for ltrim to ensure there is no issue specifically with the default extension collection functions (which have some special handling in isthmus) and an introduced ltrim function.
benbellick
force-pushed
the
ben.bellick/562/function-converter-crash
branch
from
d1a29b3 to
afa6fdf
Compare
nielspardon
reviewed
Oct 23, 2025
isthmus/src/test/java/io/substrait/isthmus/DuplicateFunctionUrnTest.java
Outdated
Show resolved
Hide resolved
nielspardon
reviewed
Oct 23, 2025
Comment on lines
+353
to
+355
| * | ||
| * Note that if there are multiple registered function extensions which can match a particular Call, | ||
| * the last one added to the extension collection will be matched. |
Member
There was a problem hiding this comment.
I'm wondering whether this comment should be also in a more prominent place for any consumers of isthmus and where they would find that. Would maybe at the top of the FunctionConverter class be more prominent?
Member
Author
There was a problem hiding this comment.
I ended up adding quite a bit of comments. Let me know if anything added is incorrect or just too much :)
Member
There was a problem hiding this comment.
looks great, I was suggesting to Victor a couple weeks in another PR that we maybe should try to add javadocs whenever we touch code to slowly build up javadoc documentation so this is a great contribution
nielspardon
reviewed
Oct 23, 2025
isthmus/src/test/java/io/substrait/isthmus/DuplicateFunctionUrnTest.java
Outdated
Show resolved
Hide resolved
nielspardon
reviewed
Oct 23, 2025
isthmus/src/test/java/io/substrait/isthmus/DuplicateFunctionUrnTest.java
Outdated
Show resolved
Hide resolved
Member
|
looks good in general. just some minor remarks from my side |
This commit: - improves the documentation around the `FunctionConverter.java` class - fixes some minor stylistic comments around the tests
nielspardon
reviewed
Oct 23, 2025
isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java
Show resolved
Hide resolved
benbellick
changed the title
FunctionConverter
benbellick
changed the title
FunctionConverterFunctionConverter with duplicate functions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #562
This test resolves an issue in which
FunctionConverters constructed withExtensionCollections that contain multiple functions with different URNs results in an exception being thrown.The fix is to allow a list of functions to be associated with a particular
<name>:<sig>pairing, and then to select the final option of those available. This is reflected in the comments, which clarify that the last added function takes precedence.