-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: support fixed size list for array reverse #16423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support fixed size list for array reverse #16423
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.
This looks good to me, just a few pointers
let value_length = array.value_length(); | ||
|
||
for row_index in 0..(array.len() as i32) { | ||
// skip the null value | ||
if array.is_null(row_index as usize) { | ||
nulls.push(false); | ||
mutable.extend(0, 0, 1); | ||
continue; | ||
} else { | ||
nulls.push(true); | ||
} | ||
|
||
let start = row_index * value_length; | ||
let end = (row_index + 1) * value_length; | ||
|
||
let mut index = end - 1; | ||
|
||
while index >= start { | ||
mutable.extend(0, index as usize, index as usize + 1); | ||
index -= 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.
This can be refactored, instead of converting to usize later we can convert this to usize first. rev
function can also be used for the reverse logic
let value_length = array.value_length(); | |
for row_index in 0..(array.len() as i32) { | |
// skip the null value | |
if array.is_null(row_index as usize) { | |
nulls.push(false); | |
mutable.extend(0, 0, 1); | |
continue; | |
} else { | |
nulls.push(true); | |
} | |
let start = row_index * value_length; | |
let end = (row_index + 1) * value_length; | |
let mut index = end - 1; | |
while index >= start { | |
mutable.extend(0, index as usize, index as usize + 1); | |
index -= 1; | |
} | |
} | |
let value_length = array.value_length() as usize; | |
for row_index in 0..array.len() { | |
// skip the null value | |
if array.is_null(row_index) { | |
if array.is_null(row_index) { | |
nulls.push(false); | |
mutable.extend(0, 0, 1); | |
continue; | |
} else { | |
nulls.push(true); | |
} | |
let start = row_index * value_length; | |
let end = start + value_length; | |
for idx in (start..end).rev() { | |
mutable.extend(0, idx, idx + 1); | |
} | |
} |
query ?? | ||
select array_reverse(arrow_cast(make_array(1, 2, 3), 'FixedSizeList(3, Int64)')), array_reverse(arrow_cast(make_array(1), 'FixedSizeList(1, Int64)')); | ||
---- | ||
[3, 2, 1] [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.
Lets add a test for null values as well
13482f3
to
4aeb100
Compare
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 @chenkovsky and @jonathanc-n for the review
Which issue does this PR close?
Rationale for this change
array_reverse doesn't support fixed size list now.
What changes are included in this PR?
support fixed size list for array_reverse
Are these changes tested?
UT
Are there any user-facing changes?
No