Support reverse for ListView #18424

vegarsti · 2025-11-01T06:38:24Z

Which issue does this PR close?

Closes Support reverse for ListView #18350.

Rationale for this change

We want to be able to reverse a ListView.

What changes are included in this PR?

Downcast &dyn Array to ListView: as_list_view_array
Downcast &dyn Array to LargeListView: as_large_list_view_array
Branches in array_reverse_inner to reverse ListView and LargeListView
Main logic in list_view_reverse which materializes a new values array using take

Are these changes tested?

Yes

datafusion/functions-nested/src/reverse.rs

vegarsti · 2025-11-01T06:56:07Z

datafusion/functions-nested/src/reverse.rs

+    }
+
+    // Take values from underlying array in the reversed order
+    let indices_array: ArrayRef = match size_of::<O>() {


I'm not sure about this, but I wanted to 1) avoid unwraps 2) don't use 64 if 32 was sufficient.

Changed to use O::IS_LARGE but still wondering about the approach

vegarsti · 2025-11-01T07:12:57Z

datafusion/functions-nested/src/reverse.rs

    )?))
 }

+fn list_view_reverse<O: OffsetSizeTrait + TryFrom<i64>>(


I have to admit I did not spend much time looking at general_array_reverse, maybe I should have... It constructs a MutableArrayData and operates on it, while this uses take. There might be a good reason why general_array_reverse doesn't use take?

Using take makes sense to me here. If you have time you could try both approaches and run a benchmark?

Good idea, I will try that

Alright, I tried the benchmark here #18425 with array_len of 10k (it's 100k on the branch), using MutableData is way worse 🫨 Baseline is the code on this PR, the code from this snippet is here.

array_reverse time: [44.858 µs 44.865 µs 44.874 µs] change: [+545.75% +547.46% +549.17%] (p = 0.00 < 0.05) Performance has regressed.

This indicates it might be worth using take instead of MutableData on the regular array one too.

Sounds good to go with take in that case

Jefffrey

Thanks for this, have some suggestions for refactoring.

I think its unavoidable to reshuffle/materialize the underlying child array given my understanding of ListView arrays.

Reference: #18350 (comment)

datafusion/functions-nested/src/reverse.rs

Jefffrey · 2025-11-02T13:11:31Z

datafusion/functions-nested/src/reverse.rs

+    // Materialize values from underlying array with take
+    let indices_array: ArrayRef = if O::IS_LARGE {
+        Arc::new(arrow::array::UInt64Array::from(
+            indices
+                .iter()
+                .map(|i| i.as_usize() as u64)
+                .collect::<Vec<_>>(),
+        ))
+    } else {
+        Arc::new(UInt32Array::from(
+            indices
+                .iter()
+                .map(|i| i.as_usize() as u32)
+                .collect::<Vec<_>>(),
+        ))
+    };


Hmm I do wonder if there is a better way to do this, will keep thinking 🤔

Yeah, this feels cumbersome!

Jefffrey · 2025-11-02T13:12:14Z

datafusion/functions-nested/src/reverse.rs

    )?))
 }

+fn list_view_reverse<O: OffsetSizeTrait + TryFrom<i64>>(


Using take makes sense to me here. If you have time you could try both approaches and run a benchmark?

## Which issue does this PR close?  N/A ## Rationale for this change  When reviewing #18424 I noticed some refactoring that could be applied to existing array reverse implementation. ## What changes are included in this PR?  ## Are these changes tested?  See my comments for the refactors & justifications. Existing tests. ## Are there any user-facing changes?  No.

datafusion/functions-nested/src/reverse.rs

alamb · 2025-11-03T13:55:58Z

datafusion/functions-nested/src/reverse.rs

+    // even if the original array had elements out of order.
+    let mut indices: Vec<O> = Vec::with_capacity(values.len());
+    let mut new_sizes = Vec::with_capacity(sizes.len());
+    let mut new_offsets: Vec<O> = Vec::with_capacity(offsets.len());


FWIW you can convert the existing indices to a Vec for in place update as well -- I tried to document that a bit more in apache/arrow-rs#8771

Jefffrey

Looks good to me 👍

I think @alamb might have confused what this reverse function is doing

datafusion/functions-nested/src/reverse.rs

Jefffrey · 2025-11-04T03:32:04Z

datafusion/functions-nested/src/reverse.rs

    )?))
 }

+fn list_view_reverse<O: OffsetSizeTrait + TryFrom<i64>>(


Sounds good to go with take in that case

Jefffrey · 2025-11-04T03:32:38Z

datafusion/functions-nested/src/reverse.rs

+    }
+
+    #[test]
+    fn test_reverse_list_view_empty() -> Result<()> {


Could we also have a test case where its all nulls?

Good idea! Done in 7d39f69

# Which issue does this PR close? - related to apache/datafusion#18424 # Rationale for this change It may not be obvious how to convert certain Arrow arrays to/from Vec without copying for manipulation, so let's add an example # What changes are included in this PR? 1. Add note about zero copy arrays 2. Add examples of modifying a primitive array using zero-copy conversion to/from Vec # Are these changes tested? By CI # Are there any user-facing changes? Docs only, no functional change --------- Co-authored-by: Vegard Stikbakke <vegard.stikbakke@gmail.com>

Jefffrey · 2025-11-05T01:50:24Z

Thanks @vegarsti

## Which issue does this PR close? - Closes apache#18350. ## Rationale for this change We want to be able to reverse a ListView. ## What changes are included in this PR? - Downcast `&dyn Array` to `ListView`: `as_list_view_array` - Downcast `&dyn Array` to `LargeListView`: `as_large_list_view_array` - Branches in `array_reverse_inner` to reverse `ListView` and `LargeListView` - Main logic in `list_view_reverse` which materializes a new values array using `take` ## Are these changes tested? Yes

There's no benchmarks for `array_reverse`. I used this while working on #18424 to confirm `take` was faster than MutableData for ListView. That might be the case for other List types as well, which are currently using `MutableData`. The benchmark can be run with `cargo bench --bench array_reverse`.

## Rationale for this change Noticed while doing #18424 that the list types `List` and `FixedSizeList` uses `MutableData` to build the reverse array. Using `take` turns out to be a lot faster, ~70% for both `List` and `FixedSizeList`. This PR also reworks the benchmark added in #18425, and these are the results on that compared to the implementation on main: ``` # cargo bench --bench array_reverse Compiling datafusion-functions-nested v50.3.0 (/Users/vegard/dev/datafusion/datafusion/functions-nested) Finished `bench` profile [optimized] target(s) in 42.08s Running benches/array_reverse.rs (target/release/deps/array_reverse-2c473eed34a53d0a) Gnuplot not found, using plotters backend Benchmarking array_reverse_list: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.3s, or reduce sample count to 70. array_reverse_list time: [62.201 ms 62.551 ms 62.946 ms] change: [−70.137% −69.965% −69.785%] (p = 0.00 < 0.05) Performance has improved. Found 8 outliers among 100 measurements (8.00%) 5 (5.00%) high mild 3 (3.00%) high severe Benchmarking array_reverse_list_view: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.3s, or reduce sample count to 70. array_reverse_list_view time: [61.649 ms 61.905 ms 62.185 ms] change: [−16.122% −15.623% −15.087%] (p = 0.00 < 0.05) Performance has improved. Found 6 outliers among 100 measurements (6.00%) 5 (5.00%) high mild 1 (1.00%) high severe array_reverse_fixed_size_list time: [4.7936 ms 4.8292 ms 4.8741 ms] change: [−76.435% −76.196% −75.951%] (p = 0.00 < 0.05) Performance has improved. Found 20 outliers among 100 measurements (20.00%) 8 (8.00%) low mild 5 (5.00%) high mild 7 (7.00%) high severe ``` ## Are these changes tested? Covered by existing sqllogic tests, and one new test for `FixedSizeList`.

## Which issue does this PR close?  N/A ## Rationale for this change  When reviewing apache#18424 I noticed some refactoring that could be applied to existing array reverse implementation. ## What changes are included in this PR?  ## Are these changes tested?  See my comments for the refactors & justifications. Existing tests. ## Are there any user-facing changes?  No.

## Which issue does this PR close? - Closes apache#18350. ## Rationale for this change We want to be able to reverse a ListView. ## What changes are included in this PR? - Downcast `&dyn Array` to `ListView`: `as_list_view_array` - Downcast `&dyn Array` to `LargeListView`: `as_large_list_view_array` - Branches in `array_reverse_inner` to reverse `ListView` and `LargeListView` - Main logic in `list_view_reverse` which materializes a new values array using `take` ## Are these changes tested? Yes

There's no benchmarks for `array_reverse`. I used this while working on apache#18424 to confirm `take` was faster than MutableData for ListView. That might be the case for other List types as well, which are currently using `MutableData`. The benchmark can be run with `cargo bench --bench array_reverse`.

## Rationale for this change Noticed while doing apache#18424 that the list types `List` and `FixedSizeList` uses `MutableData` to build the reverse array. Using `take` turns out to be a lot faster, ~70% for both `List` and `FixedSizeList`. This PR also reworks the benchmark added in apache#18425, and these are the results on that compared to the implementation on main: ``` # cargo bench --bench array_reverse Compiling datafusion-functions-nested v50.3.0 (/Users/vegard/dev/datafusion/datafusion/functions-nested) Finished `bench` profile [optimized] target(s) in 42.08s Running benches/array_reverse.rs (target/release/deps/array_reverse-2c473eed34a53d0a) Gnuplot not found, using plotters backend Benchmarking array_reverse_list: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.3s, or reduce sample count to 70. array_reverse_list time: [62.201 ms 62.551 ms 62.946 ms] change: [−70.137% −69.965% −69.785%] (p = 0.00 < 0.05) Performance has improved. Found 8 outliers among 100 measurements (8.00%) 5 (5.00%) high mild 3 (3.00%) high severe Benchmarking array_reverse_list_view: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 6.3s, or reduce sample count to 70. array_reverse_list_view time: [61.649 ms 61.905 ms 62.185 ms] change: [−16.122% −15.623% −15.087%] (p = 0.00 < 0.05) Performance has improved. Found 6 outliers among 100 measurements (6.00%) 5 (5.00%) high mild 1 (1.00%) high severe array_reverse_fixed_size_list time: [4.7936 ms 4.8292 ms 4.8741 ms] change: [−76.435% −76.196% −75.951%] (p = 0.00 < 0.05) Performance has improved. Found 20 outliers among 100 measurements (20.00%) 8 (8.00%) low mild 5 (5.00%) high mild 7 (7.00%) high severe ``` ## Are these changes tested? Covered by existing sqllogic tests, and one new test for `FixedSizeList`.

github-actions bot added the common Related to common crate label Nov 1, 2025

This was referenced Nov 1, 2025

Add benchmark for array_reverse #18425

Merged

Support reverse for ListView #18350

Closed

vegarsti commented Nov 1, 2025

View reviewed changes

datafusion/functions-nested/src/reverse.rs Outdated Show resolved Hide resolved

vegarsti commented Nov 1, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

vegarsti marked this pull request as draft November 1, 2025 08:06

vegarsti force-pushed the list-view-reverse branch from b93da9c to 1c0343d Compare November 1, 2025 18:36

vegarsti marked this pull request as ready for review November 1, 2025 18:37

Support reverse for ListView

0175167

vegarsti force-pushed the list-view-reverse branch from 1c0343d to 0175167 Compare November 1, 2025 18:42

Tidy up tests a bit

48912d9

Jefffrey reviewed Nov 2, 2025

View reviewed changes

Jefffrey mentioned this pull request Nov 2, 2025

minor: refactor array reverse internals #18445

Merged

vegarsti added 2 commits November 2, 2025 16:59

Improve with Jeffrey's suggestions

b16ce4d

Use array.is_null instead of nulls array

714f9e7

Jefffrey reviewed Nov 3, 2025

View reviewed changes

datafusion/functions-nested/src/reverse.rs Outdated Show resolved Hide resolved

datafusion/functions-nested/src/reverse.rs Outdated Show resolved Hide resolved

datafusion/functions-nested/src/reverse.rs Outdated Show resolved Hide resolved

alamb reviewed Nov 3, 2025

View reviewed changes

datafusion/functions-nested/src/reverse.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Nov 3, 2025

Add example to convert PrimitiveArray to a Vec apache/arrow-rs#8771

Merged

alamb reviewed Nov 3, 2025

View reviewed changes

vegarsti added 2 commits November 3, 2025 20:00

More improvements from Jeffrey

e72469f

Add slt test case to array.slt

71aefa9

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 3, 2025

Jefffrey approved these changes Nov 4, 2025

View reviewed changes

Add null test and remove unneeded trait

7d39f69

Jefffrey added this pull request to the merge queue Nov 5, 2025

Merged via the queue into apache:main with commit b52a81d Nov 5, 2025
28 checks passed

vegarsti deleted the list-view-reverse branch November 5, 2025 07:20

vegarsti mentioned this pull request Nov 5, 2025

Make array_reverse faster for List and FixedSizeList #18500

Merged

Support reverse for ListView #18424

Support reverse for ListView #18424

Uh oh!

Conversation

vegarsti commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vegarsti Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vegarsti Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jefffrey commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vegarsti commented Nov 1, 2025 •

edited

Loading

vegarsti Nov 1, 2025 •

edited

Loading

vegarsti Nov 3, 2025 •

edited

Loading