Rich-T-kid commented on code in PR #11770:
URL: https://github.com/apache/arrow-rs/pull/11770#discussion_r4373785956
##########
arrow/benches/coalesce_kernels.rs:
##########
@@ -1447,10 +1447,17 @@ impl DataStreamBuilder {
)
.with_timezone(Arc::clone(tz)),
),
- DataType::Utf8 => Arc::new(create_string_array::<i32>(
- self.batch_size,
- self.null_density,
- )), // TODO seed
+ // Strings of 0 to 399 bytes; `max_string_len` doesn't apply
Review Comment:
nit: this probably should be a separate PR but its not worth the effort so
its fine to keep.
##########
arrow-array/src/builder/generic_bytes_view_builder.rs:
##########
@@ -224,25 +224,39 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
/// Appends an array to the builder.
/// This will flush any in-progress block and append the data buffers
/// and add the (adapted) views.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the builder would hold more than `u32::MAX` data buffers
Review Comment:
🤔 this makes sense to do but it is a breaking change right? code the
previously worked (produced incorrect results) ran but now this will cause a
panic.
##########
arrow-select/src/coalesce/byte_view.rs:
##########
@@ -194,21 +194,32 @@ impl<B: ByteViewType> InProgressByteViewArray<B> {
self.size_of_completed_buffers_from_current_source += buffers_size;
}
- let starting_buffer: u32 = self.completed.len().try_into().expect("too
many buffers");
+ let starting_buffer = self.completed.len() as u32;
self.completed.extend_from_slice(buffers);
+ assert!(
+ u32::try_from(self.completed.len()).is_ok(),
+ "too many buffers"
+ );
if starting_buffer == 0 {
// If there are no buffers, we can just use the views as is
self.views.extend_from_slice(views);
} else {
// If there are buffers, we need to update the buffer index
let updated_views = views.iter().map(|v| {
- let mut byte_view = ByteView::from(*v);
- if byte_view.length > MAX_INLINE_VIEW_LEN {
- // Small views (<=12 bytes) are inlined, so only need to
update large views
- byte_view.buffer_index += starting_buffer;
- }
- byte_view.as_u128()
+ // A view of a string of 12 bytes or fewer stores the string
+ // inline and has no buffer index, so adding 0 leaves it
+ // unchanged.
+ let offset = if *v as u32 > MAX_INLINE_VIEW_LEN {
+ starting_buffer
+ } else {
+ 0
+ };
+ // The buffer index is bits 64 to 95 of a view. Adding
+ // `starting_buffer` gives the string's buffer index in the
+ // output, which fits in 32 bits, so the sum never carries into
+ // bit 96.
+ v + ((offset as u128) << 64)
Review Comment:
> we found that it is significantly faster to instead update the
corresponding bits in the raw view directly, via shifting and addition.
this is very neat
--
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]
