fix: GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287
Open
adriangb wants to merge 1 commit into
Open
fix: GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287adriangb wants to merge 1 commit into
GenericByteViewArray::gc() drops inline views on the multi-buffer slow path#10287adriangb wants to merge 1 commit into
Conversation
`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
Contributor
Author
|
@XiangpengHao or @rluvaton would one of you mind taking a look at this fix? |
Jefffrey
reviewed
Jul 6, 2026
Comment on lines
+581
to
+582
| // started. Cap the per-buffer reservation at `max_buffer_size` so a | ||
| // single huge value doesn't request an absurd allocation up front. |
Contributor
There was a problem hiding this comment.
i dont understand this point:
Cap the per-buffer reservation at
max_buffer_sizeso a single huge value doesn't request an absurd allocation up front.
a single huge value cannot exceed max_buffer_size (which is i32::MAX) anyway, because the length is bounded by that as well 🤔
| { | ||
| data_blocks.push(Buffer::from_vec(std::mem::take(&mut data_buf))); | ||
| current_buffer_idx += 1; | ||
| data_buf.reserve(buffer_cap); |
Contributor
There was a problem hiding this comment.
by always reserving buffer_cap does that mean the last buffer will always have more memory than strictly necessary?
| let mut views_buf = Vec::with_capacity(len); | ||
| let mut data_blocks = Vec::new(); | ||
| let mut data_buf: Vec<u8> = Vec::with_capacity(buffer_cap); | ||
| let mut current_buffer_idx: i32 = 0; |
Contributor
There was a problem hiding this comment.
we could refactor current_buffer_idx away by just using data_blocks.len()
| // 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 |
Contributor
There was a problem hiding this comment.
why do we operate on u64's here instead of i32/u32?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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().