-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
RFC: Make fields of ScalarUDF non pub #8039
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.
If we like this API change, the same would need to be applied for https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.WindowUDF.html and https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.AggregateUDF.html
#[derive(Clone)] | ||
pub struct ScalarUDF { | ||
/// name | ||
pub name: String, |
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.
the key change in this PR is to remove pub
&self.signature | ||
} | ||
/// return the return type of this function given the types of the arguments | ||
pub fn return_type(&self, args: &[DataType]) -> Result<DataType> { |
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.
Rather than expose the underlying function pointer directly, I opted to handle the nuance of calling it here.
Thank you, I think it's a good idea. Now for scalar functions there are two execution paths: What do you think is a good implementation plan for the next step? |
I was thinking one next step might be to try and port a few BuiltInFunctions to be ScalarUDFs (maybe the string functions or the crypto functions like MD5) -- specifically remove them from the I think that would force is to figure out How to register these functions with the FunctionRegistry by default, and other potential touchpoints there might be I will try and write up some ideas later todya |
b6e9c31
to
c67e620
Compare
Productionized in #8079 |
Which issue does this PR close?
Part of #7110 and #7977
Rationale for this change
Over time, the number of built in functions within DataFusion grows which is challenging both because:
If is for this reason I would like to move towards only having functions defined as
ScalarUDFs
This will ensure:What changes are included in this PR?
ScalarUDF
private and add accessors.Then we can add additional fields / trait impls as proposed by @2010YOUY01
Are these changes tested?
Covered by existing tests