-
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
feat : Support for deregistering user defined functions #9239
Conversation
This looks good to me @mobley-trent -- do you think it is ready for review? |
Hey @alamb I'm done with the final touches. |
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.
Thank you @mobley-trent -- this looks great. Nicely documented and nicely tested. I had some small suggestions, but I also think they could be done as a follow on PR as well
@@ -2247,6 +2292,196 @@ mod tests { | |||
Ok(()) | |||
} | |||
|
|||
#[tokio::test] | |||
async fn register_deregister_udf() -> Result<()> { |
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.
Would it be possible to move these tests to https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/user_defined/mod.rs
Doing so would make them more discoverable as well as likely shorter as you could probably avoid having to define ScalarUDFs
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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.
Thank you @mobley-trent 🚀
I took the liberty of pushing a fix for |
Thanks again @mobley-trent |
Which issue does this PR close?
Closes #9178
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?