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

Add TypeParameters to SqlInvokedFunction #18581

Merged

Conversation

sviscaino
Copy link
Contributor

@sviscaino sviscaino commented Oct 29, 2022

The current parser SqlInvokedScalarFromAnnotationsParser.java does not pass along TypeParameter annotations to the function signature.
This PR fixes this gap so we can write SqlInvokedFunctions with type parameters.

Test plan -
mvn test -Dtest=TestAnnotationEngineForSqlInvokedScalars test

== NO RELEASE NOTES ==

@sviscaino sviscaino requested a review from a team as a code owner October 29, 2022 16:15
@sviscaino sviscaino requested a review from presto-oss October 29, 2022 16:15
@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch from 059faa3 to e537f27 Compare October 29, 2022 16:17
@kaikalur
Copy link
Contributor

@sviscaino The idea is to do the Annotation parser change in a simple PR by itself and then you can do the array functions separately.

@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch from e537f27 to faa36b2 Compare October 29, 2022 16:27
@sviscaino sviscaino changed the title Add TypeParameters to SqlInvokedFunction and generalize input types of existing SQL UDFs Add TypeParameters to SqlInvokedFunction Oct 29, 2022
@sviscaino
Copy link
Contributor Author

sviscaino commented Oct 29, 2022

@sviscaino The idea is to do the Annotation parser change in a simple PR by itself and then you can do the array functions separately.

Ok, done - I will make a separate PR to generalize the existing SQL functions after this one is merged. I updated the PR title and description.

@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch 2 times, most recently from 27da516 to 6fec300 Compare October 29, 2022 16:30
@kaikalur
Copy link
Contributor

@sviscaino The idea is to do the Annotation parser change in a simple PR by itself and then you can do the array functions separately.

Ok, done - I will make a separate PR to generalize the existing SQL functions after this one is merged. I updated the PR title and description.

Thank you!

@kaikalur
Copy link
Contributor

Also please add a unit test for this

@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch from 6fec300 to 7d0cda9 Compare October 29, 2022 18:03
@sviscaino
Copy link
Contributor Author

Also please add a unit test for this

done (there were no unit tests for SqlInvokedScalarFromAnnotationsParser so I added them)

@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch from 7d0cda9 to 59ecdc9 Compare October 29, 2022 19:22
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

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

Thank you for adding the unit test!

@kaikalur kaikalur requested a review from highker October 29, 2022 20:23
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

@kaikalur feel free to merge the PR once the comments have been addressed

@sviscaino sviscaino force-pushed the feature/add-typeparameters-sqludf branch from 59ecdc9 to ce6dc8c Compare October 31, 2022 10:21
@kaikalur kaikalur merged commit e7a5eea into prestodb:master Oct 31, 2022
@sviscaino sviscaino deleted the feature/add-typeparameters-sqludf branch October 31, 2022 15:15
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.

3 participants