From f3b42937d3349d11467a1849ea1241c8ea119b1e Mon Sep 17 00:00:00 2001 From: Neil Conway Date: Mon, 19 Oct 2026 14:18:56 -0400 Subject: [PATCH 1/3] bench: seed `coalesce_kernels` `Utf8` and `Dictionary` inputs The `Utf8` and `Dictionary(Int32, Utf8)` inputs of the `coalesce` benchmarks ignored their seed, so all 10 input batches of each held the same strings, and only the filters and indices changed from batch to batch. Each batch now draws its strings from its own seed, with the same distribution of lengths, nulls and characters as before. The other string and binary inputs already used the seed. --- arrow/benches/coalesce_kernels.rs | 32 ++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/arrow/benches/coalesce_kernels.rs b/arrow/benches/coalesce_kernels.rs index 9cdd71cdc3bc..1f0c524cb6c5 100644 --- a/arrow/benches/coalesce_kernels.rs +++ b/arrow/benches/coalesce_kernels.rs @@ -1447,10 +1447,17 @@ impl DataStreamBuilder { ) .with_timezone(Arc::clone(tz)), ), - DataType::Utf8 => Arc::new(create_string_array::( - self.batch_size, - self.null_density, - )), // TODO seed + // Strings of 0 to 399 bytes; `max_string_len` doesn't apply + DataType::Utf8 => Arc::new( + create_string_array_with_len_range_and_prefix_and_seed::( + self.batch_size, + self.null_density, + 0, + 399, + "", + seed, + ), + ), DataType::Binary => Arc::new(create_binary_array_with_len_range_and_prefix_and_seed::< i32, >( @@ -1482,11 +1489,18 @@ impl DataStreamBuilder { if key_type.as_ref() == &DataType::Int32 && value_type.as_ref() == &DataType::Utf8 => { - Arc::new(create_string_dict_array::( - self.batch_size, - self.null_density, - self.max_string_len, - )) // TODO seed + Arc::new( + create_string_array_with_len_range_and_prefix_and_seed::( + self.batch_size, + self.null_density, + self.max_string_len, + self.max_string_len, + "", + seed, + ) + .iter() + .collect::>(), + ) } DataType::FixedSizeBinary(size) => Arc::new(create_fixed_size_binary_array( self.batch_size, From 1cc93139208ac5e883c5a18466bd8e3c95687e52 Mon Sep 17 00:00:00 2001 From: Neil Conway Date: Mon, 19 Oct 2026 15:00:30 -0400 Subject: [PATCH 2/3] perf: add the buffer offset to string views with a shift in `BatchCoalescer` When `BatchCoalescer` keeps an input's string view data buffers instead of copying its strings, it adds the number of buffers already in the output to the buffer index of each view longer than 12 bytes. It unpacked each such view into a `ByteView`, added the offset to the `buffer_index` field, and packed the fields back into a `u128`. Now it picks the offset or 0 from the view's length, shifts it to the position of the buffer index, bits 64 to 95, and adds it to the whole view. The increased index is less than the number of buffers in the output, so the sum never carries into bit 96 as long as that number fits in 32 bits. The coalescer checked the number before adding the input's buffers; it now checks it after, so the check covers every increased index. `coalesce_kernels` cases that take this path, each version run twice in alternating order. The `mixed_binaryview` cases without nulls that keep 80% of rows copy their strings instead, and are left out. - AMD EPYC Milan: all 26 cases are faster, by 0.4% to 11%. Filters that keep 80% of rows: -2.5% to -10%; `take` of 80%: -1.4% to -11%; filters that keep 1%: -0.4% to -4%. Strings of 13 to 39 bytes, all stored out of line: -2% to -7.8%. - Apple M4 Max: 25 cases are faster, by 0.3% to 15%. One is 1% slower: a filter that keeps 1% of `BinaryView` rows with nulls, whose two runs were 6% apart. Filters that keep 80%: -3.2% to -7.7%; `take` of 80%: -3.9% to -15%; filters that keep 1%: -6.9% to +1%. Strings of 13 to 39 bytes: -1.4% to -7.6%. --- arrow-select/src/coalesce/byte_view.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/arrow-select/src/coalesce/byte_view.rs b/arrow-select/src/coalesce/byte_view.rs index a6697bfdb599..ca3d2280a141 100644 --- a/arrow-select/src/coalesce/byte_view.rs +++ b/arrow-select/src/coalesce/byte_view.rs @@ -194,8 +194,12 @@ impl InProgressByteViewArray { 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 @@ -203,12 +207,19 @@ impl InProgressByteViewArray { } 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) }); self.views.extend(updated_views); From d666b345e33a5a9103cfaaee0df618de42fd07a4 Mon Sep 17 00:00:00 2001 From: Neil Conway Date: Mon, 19 Oct 2026 15:00:30 -0400 Subject: [PATCH 3/3] perf: add the buffer offset to string views with a shift in `GenericByteViewBuilder::append_array` `GenericByteViewBuilder::append_array`, which `concat` uses for string and binary views, adds the number of data buffers already in the builder to the buffer index of each appended view longer than 12 bytes. It unpacked each such view into a `ByteView`, added the offset to the `buffer_index` field, and packed the fields back into a `u128`. Now it picks the offset or 0 from the view's length, shifts it to the position of the buffer index, bits 64 to 95, and adds it to the whole view. The increased index is less than the number of buffers in the builder, so the sum never carries into bit 96 as long as that number fits in 32 bits. `append_array` truncated the number to 32 bits without checking it; it now panics if the number doesn't fit, as the builder already does when it adds a single buffer. `concat` of 10 string view arrays of 8192 rows, each version run twice in alternating order: - AMD EPYC Milan: -34% to -35% when the arrays have out-of-line strings, whether all of their strings are (20 or 128 bytes) or about 40% are (3 to 19 bytes), with or without nulls. Arrays of inline strings take a path that doesn't change, and are within 0.2%. - Apple M4 Max: -11% to -12% for the same cases. Arrays of inline strings are within 0.4%. --- .../src/builder/generic_bytes_view_builder.rs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/arrow-array/src/builder/generic_bytes_view_builder.rs b/arrow-array/src/builder/generic_bytes_view_builder.rs index 6ff209ef7445..3c21b8624b99 100644 --- a/arrow-array/src/builder/generic_bytes_view_builder.rs +++ b/arrow-array/src/builder/generic_bytes_view_builder.rs @@ -224,6 +224,10 @@ impl GenericByteViewBuilder { /// 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 pub fn append_array(&mut self, array: &GenericByteViewArray) { self.flush_in_progress(); // keep original views if this array is the first to be added or if there are no data buffers (all inline views) @@ -231,18 +235,28 @@ impl GenericByteViewBuilder { let starting_buffer = self.completed.len() as u32; self.completed.extend(array.data_buffers().iter().cloned()); + assert!( + u32::try_from(self.completed.len()).is_ok(), + "Too many blocks" + ); if keep_views { self.views_buffer.extend_from_slice(array.views()); } else { self.views_buffer.extend(array.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 + // builder, which fits in 32 bits, so the sum never carries + // into bit 96. + v + ((offset as u128) << 64) })); }