mapleFU commented on code in PR #10287:
URL: https://github.com/apache/arrow-rs/pull/10287#discussion_r3569332869
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -556,67 +569,61 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
let data_blocks = vec![data_block];
(views_buf, data_blocks)
} else {
- // slow path, need to split into multiple buffers
-
- struct GcCopyGroup {
- total_buffer_bytes: usize,
- total_len: usize,
- }
-
- impl GcCopyGroup {
- fn new(total_buffer_bytes: u32, total_len: usize) -> Self {
- Self {
- total_buffer_bytes: total_buffer_bytes as usize,
- 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`),
+ // so it must be split across several buffers.
+ //
+ // Iterate over *all* views in order — this is essential for
+ // correctness, as gc must preserve the row count. 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 `max_buffer_size`, at which point a new buffer is
+ // started.
+ //
+ // `total_large` is the *sum* of all non-inline bytes — it is what
+ // pushed us onto this path, so it can be many GiB. Size each
buffer
+ // reservation to the data still to be copied, capped at one
buffer's
+ // worth (`max_buffer_size`), so we neither reserve the whole
+ // multi-GiB total up front nor pad the final buffer out to a full
+ // `max_buffer_size` when only a little data is left.
+ 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));
+
+ for i in 0..len {
+ // SAFETY: `i < len == self.len()`.
+ let view_len = *unsafe { self.views().get_unchecked(i) } as
u32;
+ let is_large = view_len > MAX_INLINE_VIEW_LEN;
+ // Only non-inline views consume space in the data buffer; if
the
+ // 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`
+ // — in that case `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.
+ if is_large
+ && !data_buf.is_empty()
+ && data_buf.len() as u64 + view_len as u64 >
max_buffer_size as u64
+ {
+ data_blocks.push(Buffer::from_vec(std::mem::take(&mut
data_buf)));
+ data_buf.reserve(remaining_large.min(max_buffer_size));
}
- }
- let mut groups = Vec::new();
- let mut current_length = 0;
- let mut current_elements = 0;
-
- for view in self.views() {
- let len = *view as u32;
- if len > MAX_INLINE_VIEW_LEN {
- if current_length + len > i32::MAX 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;
Review Comment:
So extract `current_elements += 1;` fix the bug that the small buffer being
eliminate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
