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? |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks great @Weijun-H -- thank you for the contrubution and thank you @jayzhan211 for the review
| 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 |
3 participants
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?