alamb commented on code in PR #7873:
URL: https://github.com/apache/arrow-rs/pull/7873#discussion_r2198191330
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -473,13 +473,78 @@ 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 {
- let mut builder =
GenericByteViewBuilder::<T>::with_capacity(self.len());
+ // 1) Read basic properties once
+ let len = self.len(); // number of elements
+ let views = self.views(); // raw u128 "view" values per slot
+ let nulls = self.nulls().cloned(); // reuse & clone existing null
bitmap
+
+ // 1.5) Fast path: if there are buffers, just reuse original views and
no data blocks
Review Comment:
```suggestion
// 1.5) Fast path: if there are no buffers, just reuse original
views and no data blocks
```
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -473,13 +473,78 @@ 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 {
- let mut builder =
GenericByteViewBuilder::<T>::with_capacity(self.len());
+ // 1) Read basic properties once
+ let len = self.len(); // number of elements
+ let views = self.views(); // raw u128 "view" values per slot
+ let nulls = self.nulls().cloned(); // reuse & clone existing null
bitmap
+
+ // 1.5) Fast path: if there are buffers, just reuse original views and
no data blocks
+ if self.data_buffers().is_empty() {
+ return unsafe {
+ GenericByteViewArray::new_unchecked(
+ self.views().clone(),
+ vec![], // empty data blocks
+ nulls,
+ )
+ };
+ }
- for v in self.iter() {
- builder.append_option(v);
+ // 2) Calculate total size of all non-inline data and detect if any
exists
+ let total_large = self.total_buffer_bytes_used();
+
+ // 2.5) Fast path: if there is no non-inline data, avoid buffer
allocation & processing
+ if total_large == 0 {
+ // Views are inline-only or all null; just reuse original views
and no data blocks
+ return unsafe {
+ GenericByteViewArray::new_unchecked(
+ self.views().clone(),
+ vec![], // empty data blocks
+ nulls,
+ )
+ };
}
- builder.finish()
+ // 3) Allocate exactly capacity for all non-inline data
+ let mut data_buf = Vec::with_capacity(total_large);
+
+ // 4) Iterate over views and process each inline/non-inline view
+ let views_buf: Vec<u128> = (0..len)
+ .map(|i| self.process_view(i, views, &mut data_buf))
+ .collect();
+
+ // 5) Wrap up buffers
+ let data_block = Buffer::from_vec(data_buf);
+ let views_scalar = ScalarBuffer::from(views_buf);
+ let data_blocks = vec![data_block];
+
+ // SAFETY: views_scalar, data_blocks, and nulls are correctly aligned
and sized
+ unsafe { GenericByteViewArray::new_unchecked(views_scalar,
data_blocks, nulls) }
+ }
+
+ // This is the helper function that processes a view at index `i`,
+ // extracting the data from the buffers if necessary.
+ // It used by `gc` function to process each view.
+ #[inline(always)]
+ fn process_view(&self, i: usize, views: &[u128], data_buf: &mut Vec<u8>)
-> u128 {
Review Comment:
I think we should mark this method as unsafe as it makes assumptions that
views always point to a valid view in self.buffers. It would be good to
document assumptions too:
1. `views` but be valid views that point to `self.buffers`
1. the returned view is updated to point at buffer "0" and the bytes are
copued to data_buf
I think we might be able to make it safer by *NOT* passing in views but
instead using `self.views` -- that would make it clearer that the views *MUST*
come from self (and thus refer to a valid self.buffer)
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -473,13 +473,78 @@ 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 {
- let mut builder =
GenericByteViewBuilder::<T>::with_capacity(self.len());
+ // 1) Read basic properties once
+ let len = self.len(); // number of elements
+ let views = self.views(); // raw u128 "view" values per slot
+ let nulls = self.nulls().cloned(); // reuse & clone existing null
bitmap
+
+ // 1.5) Fast path: if there are buffers, just reuse original views and
no data blocks
+ if self.data_buffers().is_empty() {
+ return unsafe {
+ GenericByteViewArray::new_unchecked(
+ self.views().clone(),
+ vec![], // empty data blocks
+ nulls,
+ )
+ };
+ }
- for v in self.iter() {
- builder.append_option(v);
+ // 2) Calculate total size of all non-inline data and detect if any
exists
+ let total_large = self.total_buffer_bytes_used();
+
+ // 2.5) Fast path: if there is no non-inline data, avoid buffer
allocation & processing
+ if total_large == 0 {
+ // Views are inline-only or all null; just reuse original views
and no data blocks
+ return unsafe {
+ GenericByteViewArray::new_unchecked(
+ self.views().clone(),
+ vec![], // empty data blocks
+ nulls,
+ )
+ };
}
- builder.finish()
+ // 3) Allocate exactly capacity for all non-inline data
+ let mut data_buf = Vec::with_capacity(total_large);
+
+ // 4) Iterate over views and process each inline/non-inline view
+ let views_buf: Vec<u128> = (0..len)
+ .map(|i| self.process_view(i, views, &mut data_buf))
+ .collect();
+
+ // 5) Wrap up buffers
+ let data_block = Buffer::from_vec(data_buf);
+ let views_scalar = ScalarBuffer::from(views_buf);
+ let data_blocks = vec![data_block];
+
+ // SAFETY: views_scalar, data_blocks, and nulls are correctly aligned
and sized
+ unsafe { GenericByteViewArray::new_unchecked(views_scalar,
data_blocks, nulls) }
+ }
+
+ // This is the helper function that processes a view at index `i`,
+ // extracting the data from the buffers if necessary.
+ // It used by `gc` function to process each view.
+ #[inline(always)]
+ fn process_view(&self, i: usize, views: &[u128], data_buf: &mut Vec<u8>)
-> u128 {
Review Comment:
I think we could name this something a bit more specific
```suggestion
fn copy_view_to_buffer(&self, i: usize, views: &[u128], data_buf: &mut
Vec<u8>) -> u128 {
```
--
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]
