Jefffrey commented on code in PR #10287:
URL: https://github.com/apache/arrow-rs/pull/10287#discussion_r3527927377
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -556,67 +569,47 @@ 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,
- }
- }
- }
-
- 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;
+ // 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. Cap the per-buffer reservation at `max_buffer_size` so
a
+ // single huge value doesn't request an absurd allocation up front.
Review Comment:
i dont understand this point:
> Cap the per-buffer reservation at `max_buffer_size` so 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 🤔
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -556,67 +569,47 @@ 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,
- }
- }
- }
-
- 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;
+ // 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. Cap the per-buffer reservation at `max_buffer_size` so
a
+ // single huge value doesn't request an absurd allocation up front.
+ let buffer_cap = total_large.min(max_buffer_size);
+ 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;
+
+ for i in 0..len {
+ // SAFETY: `i < len == self.len()`.
+ let view_len = *unsafe { self.views().get_unchecked(i) } as
u32;
+ // 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`.
+ 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
+ {
+ data_blocks.push(Buffer::from_vec(std::mem::take(&mut
data_buf)));
+ current_buffer_idx += 1;
+ data_buf.reserve(buffer_cap);
Review Comment:
by always reserving `buffer_cap` does that mean the last buffer will always
have more memory than strictly necessary?
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -556,67 +569,47 @@ 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,
- }
- }
- }
-
- 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;
+ // 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. Cap the per-buffer reservation at `max_buffer_size` so
a
+ // single huge value doesn't request an absurd allocation up front.
+ let buffer_cap = total_large.min(max_buffer_size);
+ 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;
Review Comment:
we could refactor `current_buffer_idx` away by just using `data_blocks.len()`
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -556,67 +569,47 @@ 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,
- }
- }
- }
-
- 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;
+ // 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. Cap the per-buffer reservation at `max_buffer_size` so
a
+ // single huge value doesn't request an absurd allocation up front.
+ let buffer_cap = total_large.min(max_buffer_size);
+ 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;
+
+ for i in 0..len {
+ // SAFETY: `i < len == self.len()`.
+ let view_len = *unsafe { self.views().get_unchecked(i) } as
u32;
+ // 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`.
+ 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
Review Comment:
why do we operate on `u64`'s here instead of i32/u32?
--
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]
