Skip to content
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

Merged
merged 3 commits into from
Feb 24, 2024

Conversation

bvolpato
Copy link
Member

@bvolpato bvolpato commented Feb 8, 2024

My attempt to take a stab at the TODO in the code:

  // TODO: define up-converting matchers.

We've got here since we noticed that functions that have list<any1>, any1 arguments wouldn't match list<string>, string without implementing a proper CallConverter as adapter:

  • The FunctionConverter was not matching using directMatchKey since list_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 existent isMatch logic, which relies on IgnoreNullableAndParameters internally).

--

Let me know if there are any concerns or thoughts on how to make this better.

@bvolpato bvolpato force-pushed the improve-type-matchers branch from a5adeeb to 7e2c6f8 Compare February 8, 2024 05:08
@bvolpato bvolpato changed the title Add up-converting signature matchers, and coerce types to match feat: add up-converting signature matchers, and coerce types to match Feb 8, 2024
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.

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 🧠

@bvolpato bvolpato changed the title feat: add up-converting signature matchers, and coerce types to match feat(isthmus): add up-converting signature matchers, and coerce types to match Feb 14, 2024
@bvolpato bvolpato requested a review from vbarua February 16, 2024 15:26
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.

Left some more comments.

@bvolpato bvolpato requested a review from vbarua February 21, 2024 04:43
@bvolpato bvolpato force-pushed the improve-type-matchers branch from 2719255 to b9574cb Compare February 22, 2024 01:55
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.

Thanks for adding this in!

@vbarua vbarua merged commit ec1887c into substrait-io:main Feb 24, 2024
8 checks passed
ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024
… arguments (substrait-io#226)

Add up-converting signature matchers to assist in matching functions containing `any` arguments
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.

2 participants