-
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
Port struct
to datafusion-functions
#9546
Conversation
datafusion/sql/src/expr/mod.rs
Outdated
Ok(Expr::ScalarFunction(ScalarFunction::new_udf( | ||
Arc::new(ScalarUDF::new_from_impl( | ||
datafusion_functions::core::r#struct::StructFunc::new(), | ||
)), |
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.
Here it will use StructFunc so I add datafusion-functions as the dependency.
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.
One of the goals of pulling datafusion functions out of the core is so that the built in functions aren't treated specially. Thus having the sql planner directly instantiate the function goes against that goal.
Can you please change this to call the struct()
function by name (get_function_meta
) instead of adding a dependency
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.
Fixed
learn something from it :)
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.
LGTM
Thank you @yyy1000 for helping along the 🚂 of function extraction |
struct
to datafusion-functions
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 @yyy1000 and @jayzhan211 -- this is looking great
Which issue does this PR close?
Related #9285 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?