-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: make all udf function impls public #9903
Conversation
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.
Makes sense to me -- thank you @universalmind303
I wonder if you want to try and add some sort of test to ensure these are all pub somehow? If not, I predict that as we port over additional functions some will get missed
@@ -54,11 +54,17 @@ make_udf_function!(ArrayHasAny, | |||
); | |||
|
|||
#[derive(Debug)] | |||
pub(super) struct ArrayHas { |
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.
It seems that all the arrays function are public, do you plan to public the rest of them too?
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.
Yes, this should include all of functions from functions
and functions-array
crates at the time of authoring.
Thanks @universalmind303. BTW @viirya is discussing adding a way to instantiate only some of the UDFs here #9892 |
I merged this branch up locally to main and verified clippy and tests passed. 🚀 |
* refactor: make all udf function impls public * clippy
* refactor: make all udf function impls public * clippy Co-authored-by: universalmind303 <[email protected]>
Which issue does this PR close?
Closes #9900
Rationale for this change
What changes are included in this PR?
this just makes all of the structs implementing
ScalarUDFImpl
publicly accessibleAre these changes tested?
Are there any user-facing changes?