-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: support LargeList
in array_dims
#8592
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
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!
@@ -1925,12 +1925,29 @@ pub fn array_length(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
|
|||
/// Array_dims SQL function | |||
pub fn array_dims(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
let list_array = as_list_array(&args[0])?; | |||
let data = match args[0].data_type() { |
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.
please check args not empty, especially if its pub function
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.
I will address this later in the pull request as other functions may also lack this check.
query ??? | ||
select array_dims(arrow_cast(make_array(1, 2, 3), 'LargeList(Int64)')), array_dims(arrow_cast(make_array([1, 2], [3, 4]), 'LargeList(List(Int64))')), array_dims(arrow_cast(make_array([[[[1], [2]]]]), 'LargeList(List(List(List(List(Int64)))))')); | ||
---- | ||
[3] [2, 2] [1, 1, 1, 2, 1] |
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.
👍
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.
looks great @Weijun-H please address a minor comment
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 thanks @Weijun-H
Which issue does this PR close?
Parts #8185
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?