move ArrayDims, ArrayNdims and Cardinality to datafusion-function-crate #9425
Conversation
github-actions
bot
added
logical-expr
|
@Weijun-H Can you also add a roundtrip test for those functions? |
There was a problem hiding this comment.
This looks great @Weijun-H -- thank you for the contrubution and thank you @jayzhan211 for the review
| @@ -319,3 +316,125 @@ pub fn gen_range( | |||
| )?); | |||
| Ok(arr) | |||
| } | |||
|
|
|||
| /// Returns the length of each array dimension | |||
There was a problem hiding this comment.
I know this is just following the existing pattern of array_functions, but I wonder if it would be better to organize the code by function.
For example, we could put the UDF and implementations in datafusion/functions-array/src/dims.rs 🤔
We could definitely do this as a follow on PR
Any thoughts @jayzhan211 ?
There was a problem hiding this comment.
Should we split two files udf and kernel inside dims?
There was a problem hiding this comment.
I think the difficult part is naming the category name. Like array_append, array_prepend and array_concat. Should we name it mutable? How about pop_front and pop_back.
There was a problem hiding this comment.
I agree how to organize the functions is not clear.
We could always just make one file for each function, I suppose. array_append.rs, array_prepend.rs, etc. Though that feels like a bit of an overkill
|
Looks like there is a conflict that needs to be resolved |
Weijun-H
force-pushed
the
move-array-dims-and-card
branch
from
944848b
to
e6a5c3c
Compare
|
Thanks @Weijun-H |
Which issue does this PR close?
Parts #9285
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?