Fix coalesce, struct and named_strct expr_fn function to take multiple arguments#10321
Fix coalesce, struct and named_strct expr_fn function to take multiple arguments#10321alamb merged 3 commits intoapache:mainfrom
coalesce, struct and named_strct expr_fn function to take multiple arguments#10321Conversation
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
While adding a test for coalesce I realized the other functions in core weren't covered so I added new tests for them too (except for get_field which needs a map / struct and I was too lazy to add one to the test fixture
| (get_field, arg_1 arg_2, "Returns the value of the field with the given name from the struct"), | ||
| (coalesce, args, "Returns `coalesce(args...)`, which evaluates to the value of the first expr which is not NULL") | ||
| ); | ||
| pub mod expr_fn { |
There was a problem hiding this comment.
It is not possible to create an expr_fn that takes a Vec<Expr> using the export_functions macro, so follow the model @Omega359 used in the math module
datafusion/datafusion/functions/src/math/mod.rs
Lines 78 to 270 in af39506
There was a problem hiding this comment.
I think it is possible to take Vec like what functions-array macro does
($UDF:ty, $EXPR_FN:ident, $DOC:expr , $SCALAR_UDF_FN:ident) => {
paste::paste! {
// "fluent expr_fn" style function
#[doc = $DOC]
pub fn $EXPR_FN(arg: Vec<Expr>) -> Expr {
Expr::ScalarFunction(ScalarFunction::new_udf(
$SCALAR_UDF_FN(),
arg,
))
}There was a problem hiding this comment.
What I could't figure out how to do was make the macro take both syntaxes
It would have to look something like
export_functions!(
// create a function with arg_1 and arg_2 Expr arguments
(arrow_cast, arg_1 arg_2, "returns arg_1 cast to the `arrow_type` given the second argument. This can be used to cast to a specific `arrow_type`."),
/// create a function with a single Vec<Expr> arg
(coalesce, args, "Returns `coalesce(args...)`, which evaluates to the value of the first expr which is not NULL")
...
}the macro definition is
datafusion/datafusion/functions/src/macros.rs
Lines 39 to 60 in 7c1c794
there may be some way to do this in rust, but I could't figure it out
There was a problem hiding this comment.
I spent a whole day trying to make it work as well and got lost in the depths of macro programming. I'm not sure it's worth the effort of trying to get it to work tbh - the alternative is not exactly hard work.
There was a problem hiding this comment.
datafusion/datafusion/functions-array/src/macros.rs
Lines 47 to 109 in e3487ee
In array macro, we support two syntaxes, which is surprisingly easy
There was a problem hiding this comment.
I was thinking of alamb#19.
However, it does not reuse the macro export_functions that accepts multiple functions with one macro call.
export_functions_single accepts a single function with each macro call.
Since this requires introducing another macro, it is also not an ideal solution.
I'm fine to move on with the current PR.
There was a problem hiding this comment.
https://users.rust-lang.org/t/macro-repetition-with-multiple-rules/110816/2?u=jayzhan
I got a probably better solution (I had not tried it) from rust forum. To anyone that is interested in
There was a problem hiding this comment.
Thank you @jayzhan211 -- I will give it a try
There was a problem hiding this comment.
I couldn't make this work (though I didn't try very hard) so I filed #10397 as a follow on task for this
There was a problem hiding this comment.
I think we can merge this first and release 38.
| } | ||
|
|
||
| /// Returns `coalesce(args...)`, which evaluates to the value of the first expr which is not NULL | ||
| pub fn coalesce(args: Vec<Expr>) -> Expr { |
There was a problem hiding this comment.
This is the actual fix, though I suspect that struct and named_struct are also affected
coalesce, struct and named_strct expr_fn function to take multiple arguments
appletreeisyellow
left a comment
There was a problem hiding this comment.
🧹 Thank you for fixing them! With the added tests, we have more confidence with future changes to these functions
|
Thanks @jayzhan211 |
Which issue does this PR close?
Closes #10320
Rationale for this change
There was a regression in the
coalescefunction's signature due to lack of testingWhat changes are included in this PR?
Are these changes tested?
Yes, by existing CI and new tests
Are there any user-facing changes?