mapleFU commented on PR #10287:
URL: https://github.com/apache/arrow-rs/pull/10287#issuecomment-4956281404
I wonder whether this is ok, since the wrong problem is `current_elements`
is in the if condition.
```diff
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);
```
--
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]
