-
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
Minor: Make it easier to work with Expr::ScalarFunction #8350
Conversation
@@ -362,6 +362,13 @@ pub struct ScalarFunction { | |||
pub args: Vec<Expr>, | |||
} | |||
|
|||
impl ScalarFunction { | |||
// return the Function's name | |||
pub fn name(&self) -> &str { |
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 point of the PR is to add this
@@ -1219,8 +1226,8 @@ impl fmt::Display for Expr { | |||
write!(f, " NULLS LAST") | |||
} | |||
} | |||
Expr::ScalarFunction(ScalarFunction { func_def, args }) => { | |||
fmt_function(f, func_def.name(), false, args, true) | |||
Expr::ScalarFunction(fun) => { |
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.
Doing so I think makes code like this easier / less inceptionish (Expr::ScalarFunction vs expr::ScalarFunction 🤯 )
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.
great small improvement. I was actually going to open up an issue to expose the name
as a method. I'm currently working on an unrelated PR that is manually mapping the variants to the names, but this'll make it much easier & cleaner.
Thanks @universalmind303 |
Which issue does this PR close?
Related to #8258
Rationale for this change
While integrating the change from #8258 downstream in IOx, I found it would have been very helpful to have
fun.name()
available and I think it makes the code more consiseWhat changes are included in this PR?
Add
expr::ScalarFunction::name()
Are these changes tested?
Existing tests
Are there any user-facing changes?