Move make_array, array_{intersect, union, distinct} to datafusion-functions-array`
#9222
Conversation
alamb
left a comment
There was a problem hiding this comment.
Here is a start on moving array functions -- I still need to work on this a bit more. FYI @jayzhan211
| Ok(()) | ||
| } | ||
|
|
||
| /// Convert one or more [`ArrayRef`] of the same type into a |
There was a problem hiding this comment.
I moved the code from here to the datafusion-function-array crate
| Sqrt = 17; | ||
| Tan = 18; | ||
| Trunc = 19; | ||
| Array = 20; |
There was a problem hiding this comment.
array is an alias for make_array it seems
| ); | ||
| } | ||
|
|
||
| fn align_array_dimensions<O: OffsetSizeTrait>( |
There was a problem hiding this comment.
I copied this over from array_expressions where it is used in one other place. I'll try and figure out how to remove it shortly
| @@ -1,254 +0,0 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
I moved this code to to_string.rs to align with keeping the ScalarFunction definitions inline with their implementations
| use std::any::Any; | ||
|
|
||
| // Create static instances of ScalarUDFs for each function | ||
| make_udf_function!(ArrayToString, |
There was a problem hiding this comment.
moved to to_string.rs
| } | ||
|
|
||
| // TODO figure out how to generalize this so it isn't hard coded by name | ||
| match (fn_name(left), fn_name(right)) { |
There was a problem hiding this comment.
I am not sure why we have specific rewrites here for functions (that can't be done by constant folding or some other method). I don't like special casing the names
I need to look into it
There was a problem hiding this comment.
@Weijun-H do you happen to remember the rationale for this code?
| BuiltinScalarFunction::MakeArray, | ||
| values, | ||
| ))) | ||
| let make_array = self |
There was a problem hiding this comment.
This is interesting -- now the SQL planner needs to replace the array literals with a scalar function, but it does the lookup by name. Rather than having this string hard coded maybe there should be a list of such functions somewhere 🤔
| make_array, | ||
| array, | ||
| "returns an Arrow array using the specified input expressions.", | ||
| udf |
There was a problem hiding this comment.
Should this be make_array?
There was a problem hiding this comment.
I struggled to come up with a good name. The macro is basically creating two functions
/// fluent expr style
fn make_array(array: Expr) -> Expr {
}
/// return a ScalarUDF
fn udf() -> ScalarUdf {
...
}The udf one is used to register the function:
/// Registers all enabled packages with a [`FunctionRegistry`]
pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
let functions: Vec<Arc<ScalarUDF>> = vec![
to_string::udf(),
make_array::udf(),
set_ops::union_udf(),
set_ops::intersect_udf(),
set_ops::distinct_udf(),
];So it can't be the expr_fn name , but udf is also pretty generic 🤔
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
We should name it make_array_udf or something else including a function name since we might not be able to reuse udf or make_udf for other array functions. Prefixing it with array_function seems like a better choice.
Draft while I figure out:
Which issue does this PR close?
Part of #8045
re #9100
Rationale for this change
Set the pattern for how more array functions can be moved from
datafusiontodatafusion-functions-arraycratesWhat changes are included in this PR?
make_array,array_intersect,array_unionandarray_distinctfunction to datafusion-functions-arrayAre these changes tested?
Yes, by existing tests
Are there any user-facing changes?
No