Fix: ViewType gc on huge batch would produce bad output #8694

mapleFU · 2025-10-23T15:13:05Z

Which issue does this PR close?

Closes Array: ViewType gc() has bug when array sum length exceed i32::MAX #8681.

Rationale for this change

Previously, gc() will produce a single buffer. However, for buffer size greater than 2GiB, it would be buggy, since buffer-offset it's a 4-byte signed integer.

What changes are included in this PR?

Add a GcCopyGroup type, and do gc for it.

Are these changes tested?

Yes

Are there any user-facing changes?

gc would produce more buffers

mapleFU · 2025-10-26T13:21:48Z

arrow-array/src/array/byte_view_array.rs

-            .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) })
-            .collect();
+            for view in self.views() {
+                let len = *view as u32;


This part is so slow, but it's right, I can make it faster(by handling the numbers via grouping or batching) if required

I am not sure how you would make this much faster - I think the code needs to find the locations to split in any event

Even if the buffer size is greater than i32::MAX, it's possible that a single buffer is much smaller than i32::MAX, so this can find batch-by-batch, rather than just adding small buffer one-by-one?

mapleFU · 2025-10-26T13:22:19Z

arrow-array/src/array/byte_view_array.rs

    }

+    #[test]
+    fn test_gc_huge_array() {


This test requires about 5GiB memory, it's huge, I don't know would it affect the testing on some machines

Previous code would meet bug only when buffer greater than 4GiB, the current code can be tested when > 2GiB. Personally I think leave 2GiB for test is ok but 4GiB is also ok to me, decide on reviewer's idea.

I ran this test on my mac M3 and it takes 1.5 seconds so I think it is ok

running 1 test test array::byte_view_array::tests::test_gc_huge_array ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 578 filtered out; finished in 1.58s

I also verified that without the code in this PR, the test fails like:

---- array::byte_view_array::tests::test_gc_huge_array stdout ---- thread 'array::byte_view_array::tests::test_gc_huge_array' panicked at arrow-array/src/array/byte_view_array.rs:1444:9: assertion `left != right` failed: gc with huge buffer should not consolidate data into a single buffer left: 1 right: 1 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I have removed the single buffer limitation :-(

mapleFU · 2025-10-26T13:23:44Z

@alamb Besides, I meet this bug when I have 4GiB StringViewArray, arrow-rs regard offset as u32, however, in arrow standard, this uses i32. So I limit it to 2GiB

There're other places uses u32::MAX in view handling, should I also fix them in other patch?

mapleFU · 2025-10-26T13:27:06Z

arrow-array/src/array/byte_view_array.rs

+            };
+            vec![gc_copy_group]
+        };
+        assert!(gc_copy_groups.len() <= i32::MAX as usize);


This assertion can be removed, I just ensure it would pass

May be change to assert debug here.

alamb

Thank you @mapleFU -- this is a good find. I left some comments, let me know what you think

cc @zhuqi-lucas perhaps you have some thoughts

alamb · 2025-10-28T19:19:07Z

arrow-array/src/array/byte_view_array.rs

    }

+    #[test]
+    fn test_gc_huge_array() {


I ran this test on my mac M3 and it takes 1.5 seconds so I think it is ok

running 1 test test array::byte_view_array::tests::test_gc_huge_array ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 578 filtered out; finished in 1.58s

I also verified that without the code in this PR, the test fails like:

---- array::byte_view_array::tests::test_gc_huge_array stdout ---- thread 'array::byte_view_array::tests::test_gc_huge_array' panicked at arrow-array/src/array/byte_view_array.rs:1444:9: assertion `left != right` failed: gc with huge buffer should not consolidate data into a single buffer left: 1 right: 1 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

alamb · 2025-10-28T19:23:59Z

arrow-array/src/array/byte_view_array.rs

+                total_buffer_bytes: total_large,
+                total_len: len,
+            };
+            vec![gc_copy_group]


I know it is probably a small thing, but could you please avoid this new allocation for the common case where there is a single buffer? Perhaps by creating the data bufs directly via a function call rather than a loop over the array

This makes sense, a fixed-sized slice on stack might be better in this scenerio

alamb · 2025-10-28T19:25:12Z

arrow-array/src/array/byte_view_array.rs

-            .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) })
-            .collect();
+            for view in self.views() {
+                let len = *view as u32;


I am not sure how you would make this much faster - I think the code needs to find the locations to split in any event

alamb · 2025-10-28T19:27:13Z

arrow-array/src/array/byte_view_array.rs


-        // 3) Allocate exactly capacity for all non-inline data
-        let mut data_buf = Vec::with_capacity(total_large);
+        struct GcCopyGroup {


I found this somewhat confusing at first as it is just deferring the creation of the view buffers.

I think the code would be clearer (and faster) if you simply created the new buffers directly (with a branch for when the total length was too large)

I think this would making the fast path slower 🤔, a single copy-group is just as simple as previous code. Maybe I should just remove the allocation here ( https://github.com/apache/arrow-rs/pull/8694/files#r2470787503 )?

Does the struct will affect some performance? We can compare the benchmark.

alamb · 2025-10-28T19:29:50Z

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.14.0-1017-gcp #18~24.04.1-Ubuntu SMP Tue Sep 23 17:51:44 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing gc-fixing-huge-batch-bug (142284c) to a7572eb diff
BENCH_NAME=view_types
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench view_types
BENCH_FILTER=
BENCH_BRANCH_NAME=gc-fixing-huge-batch-bug
Results will be posted here when complete

alamb · 2025-10-28T19:30:58Z

The MIRI test is probably failing due to the massive memory use in https://github.com/apache/arrow-rs/actions/runs/18818674867/job/53690752815?pr=8694

I suggest we don't run that test under miri by disabling it, with something like

    #[cfg_attr(miri, ignore)] // Takes too long

For example

https://github.com/apache/arrow-rs/blob/ba22a214b3c469da5466f60a74ab201a268bc0fc/arrow-array/src/array/boolean_array.rs#L789-L788

alamb · 2025-10-28T19:33:25Z

🤖: Benchmark completed

Details

group                                             gc-fixing-huge-batch-bug               main
-----                                             ------------------------               ----
gc view types all without nulls[100000]           1.05  1600.4±85.77µs        ? ?/sec    1.00  1527.4±58.01µs        ? ?/sec
gc view types all without nulls[8000]             1.07     68.0±3.35µs        ? ?/sec    1.00     63.5±1.04µs        ? ?/sec
gc view types all[100000]                         1.16    296.3±9.94µs        ? ?/sec    1.00    255.3±1.11µs        ? ?/sec
gc view types all[8000]                           1.14     21.5±0.32µs        ? ?/sec    1.00     18.9±0.11µs        ? ?/sec
gc view types slice half without nulls[100000]    1.06   552.7±15.42µs        ? ?/sec    1.00    520.1±7.45µs        ? ?/sec
gc view types slice half without nulls[8000]      1.06     28.9±0.20µs        ? ?/sec    1.00     27.2±0.15µs        ? ?/sec
gc view types slice half[100000]                  1.15    145.5±3.36µs        ? ?/sec    1.00    126.3±0.63µs        ? ?/sec
gc view types slice half[8000]                    1.14     11.0±0.12µs        ? ?/sec    1.00      9.6±0.05µs        ? ?/sec
view types slice                                  1.00    709.6±1.75ns        ? ?/sec    1.00    706.8±1.32ns        ? ?/sec

zhuqi-lucas

Nice finding, thank you!

zhuqi-lucas · 2025-10-29T06:09:06Z

arrow-array/src/array/byte_view_array.rs

+            };
+            vec![gc_copy_group]
+        };
+        assert!(gc_copy_groups.len() <= i32::MAX as usize);


May be change to assert debug here.

zhuqi-lucas · 2025-10-29T06:10:30Z

arrow-array/src/array/byte_view_array.rs


-        // 3) Allocate exactly capacity for all non-inline data
-        let mut data_buf = Vec::with_capacity(total_large);
+        struct GcCopyGroup {


Does the struct will affect some performance? We can compare the benchmark.

zhuqi-lucas · 2025-10-29T06:11:50Z

arrow-array/src/array/byte_view_array.rs

+            total_len: usize,
+        }
+
+        let gc_copy_groups = if total_large > i32::MAX as usize {


If we can add such as cold flag, since it's rare for the case?

sounds ok to me ( But it's not in a loop so I think the improvement will not too much)

Ok, got it.

mapleFU · 2025-10-29T06:15:36Z

arrow-array/src/array/byte_view_array.rs

    ///   inside one of `self.buffers`.
    /// - `data_buf` must be ready to have additional bytes appended.
    /// - After this call, the returned view will have its
    ///   `buffer_index` reset to `0` and its `offset` updated so that it points


Suggested change

/// `buffer_index` reset to `0` and its `offset` updated so that it points

/// `buffer_index` reset to `buffer_index` user pass in and its `offset` updated so that it points

Add unittest

0482a20

github-actions bot added the arrow Changes to the arrow crate label Oct 23, 2025

mapleFU added 2 commits October 26, 2025 21:12

Pass the test

7034580

Make the code prettier

464db03

mapleFU requested a review from alamb October 26, 2025 13:20

mapleFU marked this pull request as ready for review October 26, 2025 13:20

mapleFU commented Oct 26, 2025

View reviewed changes

mapleFU changed the title ~~View: Fixing gc on huge batch~~ Fix: ViewType gc on huge batch would produce bad output Oct 26, 2025

mapleFU commented Oct 26, 2025

View reviewed changes

Minor change

142284c

alamb reviewed Oct 28, 2025

View reviewed changes

zhuqi-lucas approved these changes Oct 29, 2025

View reviewed changes

mapleFU commented Oct 29, 2025

View reviewed changes

	/// `buffer_index` reset to `0` and its `offset` updated so that it points
	/// `buffer_index` reset to `buffer_index` user pass in and its `offset` updated so that it points

Uh oh!

Fix: ViewType gc on huge batch would produce bad output #8694

Are you sure you want to change the base?

Fix: ViewType gc on huge batch would produce bad output #8694

Conversation

mapleFU commented Oct 23, 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?

Are there any user-facing changes?

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!

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!

mapleFU commented Oct 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Oct 29, 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!

alamb commented Oct 28, 2025

Uh oh!

alamb commented Oct 28, 2025

Uh oh!

alamb commented Oct 28, 2025

Uh oh!

zhuqi-lucas left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mapleFU commented Oct 23, 2025 •

edited

Loading

mapleFU Oct 29, 2025 •

edited

Loading