fix: GenericByteViewArray::gc() drops inline views on the multi-buffer slow path

[adriangb]

Which issue does this PR close?

N/A — filing alongside; happy to open a tracking issue if preferred.

Rationale for this change

GenericByteViewArray::gc() takes a multi-buffer slow path once a Utf8View/BinaryView column references more than i32::MAX (~2.1 GiB) of non-inline data. Because a single Arrow data buffer is addressed by a u32 offset, the compacted output has to be split across multiple buffers there.

That slow path built its copy groups by counting only non-inline views (current_elements), then copied a contiguous index range of that size. As a result:

• every inline (short) view was dropped, so the output array was shorter than its input, and
• the index-range / byte-budget mismatch could also let a single output buffer grow past i32::MAX.

gc() is documented as a pure size optimisation and must never change the row count. Downstream code that rebuilds a RecordBatch from the GC'd columns therefore panicked on the "all columns must have the same row count" invariant whenever a view column crossed the ~2.1 GiB threshold.

The existing test_gc_huge_array missed this because all of its views are non-inline, so the miscount happened to equal len.

What changes are included in this PR?

• Rewrite the slow path as a single pass over all views: inline views are copied unchanged (they reference no buffer); non-inline views are packed into the current output buffer until adding the next one would exceed the max buffer size, at which point a new buffer is sealed. This preserves the row count and value order, and keeps each output buffer within the size limit.
• Extract the body into a private gc_with_max_buffer_size(max_buffer_size); gc() calls it with i32::MAX. This lets the split path be tested with a tiny threshold instead of allocating 2+ GiB.
• Add test_gc_slow_path_preserves_inline_views, which interleaves inline, large, and null views and drives the split via a small max_buffer_size, asserting the row count, per-value equality, buffer-size cap, and that the split actually occurred.

Are there any user-facing changes?

No. Public API is unchanged; this is a correctness fix to gc().

[adriangb]

@XiangpengHao or @rluvaton would one of you mind taking a look at this fix?

[Jefffrey]

why do we operate on u64's here instead of i32/u32?

[adriangb]

Left a comment in the code, copying here for visibility: data_buf.len() itself can exceed max_buffer_size, so the sum is computed in u64 to avoid overflowing the u32 offset space on the addition.

[Jefffrey]

data_buf.len() cannot exceed max_buffer_len here, i think you mean data_buf.len() + view_len can exceed it?

[adriangb]

@Jefffrey thank you for the review. I reviewed the changes Claude made to address it and they make sense to me, let me know if you agree.

[Jefffrey]

technically i think buffers can exceed i32::MAX safely; if we fill i32::MAX - 1 of the buffer, then the last string is also a massive string (e.g. i32::MAX) then its offset is still within i32 range 🤔

that is, things are fine so long as the starting offset is within i32 range, even if its length causes the buffer to exceed i32::MAX in length

[Jefffrey]

though i do wonder if having such strings could cause problems downstream in other functions anyway 🤔

[Jefffrey]

the way of reserving this much capacity up front can lead to fragmentation with unused capacity at the end; i guess this is why the initial approach calculated groups to get exact sizes 🤔

[mapleFU]

Yes, visiting one pass is lightweight, so I uses group here

[Jefffrey]

can we clean up these comments; it should not be possible for a single value to be larger than max_buffer_size (unless we're doing testing where max_buffer_size < i32::MAX)

[Jefffrey]

data_buf.len() cannot exceed max_buffer_len here, i think you mean data_buf.len() + view_len can exceed it?

[Jefffrey]

we can probably push this remaining_large adjustment inside the same if block above where we allocate a new data_buf; this way we just subtract based on previous data_buf instead of on each iteration

[mapleFU]

So extract current_elements += 1; fix the bug that the small buffer being eliminate?

[mapleFU]

I wonder whether this is ok, since the wrong problem is current_elements is in the if condition.

diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs
index be6e799221..cf137b4faf 100644
--- a/arrow-array/src/array/byte_view_array.rs
+++ b/arrow-array/src/array/byte_view_array.rs
@@ -469,6 +469,13 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
         }
     }
 
+    pub fn gc(&self) -> Self {
+        // A single Arrow data buffer is addressed by a `u32` offset, so no
+        // output buffer may exceed `i32::MAX` bytes. Above that the data is
+        // split across multiple buffers (the "slow path").
+        self.gc_with_max_buffer_size(i32::MAX as usize)
+    }
+
     /// Returns a "compacted" version of this array
     ///
     /// The original array will *not* be modified
@@ -512,7 +519,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
     ///
     /// Note: this function does not attempt to canonicalize / deduplicate values. For this
     /// feature see  [`GenericByteViewBuilder::with_deduplicate_strings`].
-    pub fn gc(&self) -> Self {
+    pub fn gc_with_max_buffer_size(&self, max_buffer_size: usize) -> Self {
         // 1) Read basic properties once
         let len = self.len(); // number of elements
         let nulls = self.nulls().cloned(); // reuse & clone existing null bitmap
@@ -543,7 +550,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
             };
         }
 
-        let (views_buf, data_blocks) = if total_large < i32::MAX as usize {
+        let (views_buf, data_blocks) = if total_large < max_buffer_size as usize {
             // fast path, the entire data fits in a single buffer
             // 3) Allocate exactly capacity for all non-inline data
             let mut data_buf = Vec::with_capacity(total_large);
@@ -579,20 +586,20 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
             for view in self.views() {
                 let len = *view as u32;
                 if len > MAX_INLINE_VIEW_LEN {
-                    if current_length + len > i32::MAX as u32 {
+                    if current_length + len > max_buffer_size as u32 {
                         // Start a new group
                         groups.push(GcCopyGroup::new(current_length, current_elements));
                         current_length = 0;
                         current_elements = 0;
                     }
                     current_length += len;
-                    current_elements += 1;
                 }
+                current_elements += 1;
             }
             if current_elements != 0 {
                 groups.push(GcCopyGroup::new(current_length, current_elements));
             }
-            debug_assert!(groups.len() <= i32::MAX as usize);
+            debug_assert!(groups.len() <= max_buffer_size);
 
             // 3) Copy the buffers group by group
             let mut views_buf = Vec::with_capacity(len);