-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(isthmus): add up-converting signature matchers, and coerce types to match #226
Conversation
a5adeeb
to
7e2c6f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started taking a look and left some minor feedback. I can look at this some more tomorrow. I need to be fresher to really grok the type matching code 🧠
isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more comments.
isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java
Outdated
Show resolved
Hide resolved
isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java
Outdated
Show resolved
Hide resolved
isthmus/src/test/java/io/substrait/isthmus/CustomFunctionTest.java
Outdated
Show resolved
Hide resolved
2719255
to
b9574cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this in!
… arguments (substrait-io#226) Add up-converting signature matchers to assist in matching functions containing `any` arguments
My attempt to take a stab at the
TODO
in the code:We've got here since we noticed that functions that have
list<any1>, any1
arguments wouldn't matchlist<string>, string
without implementing a properCallConverter
as adapter:The
FunctionConverter
was not matching usingdirectMatchKey
sincelist_any
!=list_str
.When falling back to the
leastRestrictive
logic,list<string>
+string
would also not agree, so no functions were returned.I've kept the
leastRestrictive
logic intact, but added another fallback matcher that checks if all arguments will match (using the existentisMatch
logic, which relies on IgnoreNullableAndParameters internally).--
Let me know if there are any concerns or thoughts on how to make this better.