Skip to content

feat: support fixed size list for array reverse#16423

Merged
comphead merged 3 commits into
apache:mainfrom
chenkovsky:feature/array_reverse_fixed_size_list
Jun 18, 2025
Merged

feat: support fixed size list for array reverse#16423
comphead merged 3 commits into
apache:mainfrom
chenkovsky:feature/array_reverse_fixed_size_list

Conversation

@chenkovsky
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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);
}
}

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
Copy Markdown
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
Copy Markdown
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