fix: GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287
fix: GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287adriangb wants to merge 2 commits into
GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287Conversation
`gc()` takes a multi-buffer "slow path" once a `Utf8View`/`BinaryView` column references more than `i32::MAX` (~2.1 GiB) of non-inline data (a single Arrow buffer is addressed by a `u32` offset, so the output must be split across buffers). That path built its copy groups by counting only *non-inline* views and then copied a contiguous index range of that size, so every inline (short) view was dropped and the row count shrank. The index-range / byte-budget mismatch could also push a single output buffer past `i32::MAX`. `gc()` is a pure size optimisation and must never change the row count, so callers that rebuild a `RecordBatch` from the result tripped the row-count invariant and panicked. Replace the grouping logic with a single pass over *all* views: inline views are copied unchanged, non-inline views are packed into the current buffer until the next would exceed the max buffer size, then a new buffer is sealed. Extract the body into a private `gc_with_max_buffer_size` so the split path can be tested with a tiny threshold instead of allocating 2+ GiB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RQ5ToRQyLENghZVTbA36xt
|
@XiangpengHao or @rluvaton would one of you mind taking a look at this fix? |
| // progress even for a single value larger than `max_buffer_size`. | ||
| if view_len > MAX_INLINE_VIEW_LEN | ||
| && !data_buf.is_empty() | ||
| && data_buf.len() as u64 + view_len as u64 > max_buffer_size as u64 |
There was a problem hiding this comment.
why do we operate on u64's here instead of i32/u32?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
data_buf.len() cannot exceed max_buffer_len here, i think you mean data_buf.len() + view_len can exceed it?
- Remove the redundant current_buffer_idx; use data_blocks.len() as the index of the buffer currently being filled (per review). - Size each buffer reservation to the data still to be copied (remaining_large.min(max_buffer_size)) instead of always reserving a full max_buffer_size, so the final buffer is no longer padded out to a full reservation when little data is left. - Clarify the comments: the reservation cap is about total_large (the multi-GiB sum that forced the slow path), not a single value; and note why the buffer-overflow check is done in u64 (a single value larger than max_buffer_size can push data_buf.len() past max_buffer_size). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xx1J3hcCvDUxCMnizMn8cz
|
@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. |
| total_len, | ||
| } | ||
| // slow path: the non-inline data does not fit in a single buffer | ||
| // (a buffer offset is a `u32`, so no buffer may exceed `i32::MAX`), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
though i do wonder if having such strings could cause problems downstream in other functions anyway 🤔
| let mut remaining_large = total_large; | ||
| let mut views_buf = Vec::with_capacity(len); | ||
| let mut data_blocks: Vec<Buffer> = Vec::new(); | ||
| let mut data_buf: Vec<u8> = Vec::with_capacity(remaining_large.min(max_buffer_size)); |
There was a problem hiding this comment.
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 🤔
| // next one would overflow the current buffer's offset space, | ||
| // seal it and start a new one. The `!data_buf.is_empty()` guard | ||
| // avoids emitting a leading empty buffer and guarantees forward | ||
| // progress even for a single value larger than `max_buffer_size` |
There was a problem hiding this comment.
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)
| // progress even for a single value larger than `max_buffer_size`. | ||
| if view_len > MAX_INLINE_VIEW_LEN | ||
| && !data_buf.is_empty() | ||
| && data_buf.len() as u64 + view_len as u64 > max_buffer_size as u64 |
There was a problem hiding this comment.
data_buf.len() cannot exceed max_buffer_len here, i think you mean data_buf.len() + view_len can exceed it?
| let new_view = unsafe { self.copy_view_to_buffer(i, buffer_idx, &mut data_buf) }; | ||
| views_buf.push(new_view); | ||
| if is_large { | ||
| remaining_large -= view_len as usize; |
There was a problem hiding this comment.
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
3 participants
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 aUtf8View/BinaryViewcolumn references more thani32::MAX(~2.1 GiB) of non-inline data. Because a single Arrow data buffer is addressed by au32offset, 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:i32::MAX.gc()is documented as a pure size optimisation and must never change the row count. Downstream code that rebuilds aRecordBatchfrom 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_arraymissed this because all of its views are non-inline, so the miscount happened to equallen.What changes are included in this PR?
gc_with_max_buffer_size(max_buffer_size);gc()calls it withi32::MAX. This lets the split path be tested with a tiny threshold instead of allocating 2+ GiB.test_gc_slow_path_preserves_inline_views, which interleaves inline, large, and null views and drives the split via a smallmax_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().