Skip to content

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

Merged

Conversation

chenkovsky
Copy link
Contributor

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

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 16, 2025
Copy link
Contributor

@jonathanc-n jonathanc-n left a 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

Comment on lines 196 to 217
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;
}
}
Copy link
Contributor

@jonathanc-n jonathanc-n Jun 16, 2025

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

Suggested change
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]

Copy link
Contributor

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

@chenkovsky chenkovsky force-pushed the feature/array_reverse_fixed_size_list branch from 13482f3 to 4aeb100 Compare June 16, 2025 23:53
Copy link
Contributor

@comphead comphead left a 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

@comphead comphead merged commit 5ebc49d into apache:main Jun 18, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants