From 5392bac3c15c98093377ef7020889ad30ce373ce Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 20:09:03 +0000 Subject: [PATCH] perf(array): bulk-append paths for DynVarBinBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DynVarBinBuilder::append_zeros`, `append_n_values`, and `append_varbinview` each looped over a per-element append that re-read the `DynOffsets` enum for every row. Resolve the offset width once per call and append through a monomorphized `VarBinBuilder`. Two of the loops also did avoidable per-element work: - `append_zeros` pushed `n` empty values one at a time. Add `VarBinBuilder::append_n_empty`, which repeats the previous offset with `push_n` and appends validity with `append_n`. - `append_varbinview` called `VarBinViewData::bytes_at` per row, which re-derives the views slice and returns a refcounted `ByteBuffer`. Read the views slice once and borrow each view's payload instead. The enum hoist alone measured neutral; the wins come from the bulk offset writes and from dropping the per-row `ByteBuffer`. Medians from a local divan benchmark over the three append paths (not included in this change): | bench | before | after | | --- | --- | --- | | `append_zeros/1024` | 4.073 µs | 139.7 ns | | `append_zeros/65536` | 264.1 µs | 8.700 µs | | `append_n_values/1024` | 7.349 µs | 5.647 µs | | `append_n_values/65536` | 1.222 ms | 408.3 µs | | `append_varbinview/no_nulls/1024` | 58.71 µs | 9.623 µs | | `append_varbinview/no_nulls/65536` | 4.952 ms | 1.522 ms | | `append_varbinview/nulls/1024` | 48.15 µs | 9.300 µs | | `append_varbinview/nulls/65536` | 3.538 ms | 1.339 ms | Signed-off-by: "Robert" --- vortex-array/src/arrays/varbin/builder.rs | 165 +++++++++++++++++----- 1 file changed, 130 insertions(+), 35 deletions(-) diff --git a/vortex-array/src/arrays/varbin/builder.rs b/vortex-array/src/arrays/varbin/builder.rs index 9a0a2a8922f..fc21fdc2f10 100644 --- a/vortex-array/src/arrays/varbin/builder.rs +++ b/vortex-array/src/arrays/varbin/builder.rs @@ -15,6 +15,7 @@ use vortex_error::vortex_panic; use vortex_mask::Mask; use crate::ArrayRef; +use crate::ArrayView; use crate::Canonical; use crate::ExecutionCtx; use crate::IntoArray; @@ -27,6 +28,7 @@ use crate::arrays::VarBinView; use crate::arrays::varbin::VarBinArrayExt; use crate::arrays::varbin::VarBinArraySlotsExt; use crate::arrays::varbinview::VarBinViewArrayExt; +use crate::arrays::varbinview::build_views::BinaryView; use crate::builders::ArrayBuilder; use crate::dtype::DType; use crate::dtype::IntegerPType; @@ -99,6 +101,30 @@ impl VarBinBuilder { self.validity.append_n(false, n); } + /// Appends `n` non-null empty values. + /// + /// Every appended value has the same start and end offset, so no bytes are written. + #[inline] + pub fn append_n_empty(&mut self, n: usize) { + self.offsets.push_n(self.offsets[self.offsets.len() - 1], n); + self.validity.append_n(true, n); + } + + /// Appends the same non-null value `n` times. + #[inline] + pub fn append_n_values(&mut self, value: impl AsRef<[u8]>, n: usize) { + let slice = value.as_ref(); + if slice.is_empty() { + return self.append_n_empty(n); + } + + self.offsets.reserve(n); + self.data.reserve(slice.len().saturating_mul(n)); + for _ in 0..n { + self.append_value(slice); + } + } + #[inline] pub fn append_values

( &mut self, @@ -272,9 +298,9 @@ impl DynVarBinBuilder { /// Appends the same non-null value `n` times. pub fn append_n_values(&mut self, value: impl AsRef<[u8]>, n: usize) { - let value = value.as_ref(); - for _ in 0..n { - self.append_value(value); + match &mut self.storage { + DynOffsets::I32(builder) => builder.append_n_values(value, n), + DynOffsets::I64(builder) => builder.append_n_values(value, n), } } @@ -303,7 +329,7 @@ impl DynVarBinBuilder { /// Appends an existing [`VarBinArray`] without constructing views. pub fn append_varbin( &mut self, - array: crate::ArrayView<'_, VarBin>, + array: ArrayView<'_, VarBin>, ctx: &mut ExecutionCtx, ) -> VortexResult<()> { let offsets = array.offsets().clone().execute::(ctx)?; @@ -329,28 +355,15 @@ impl DynVarBinBuilder { /// Appends a [`VarBinViewArray`](crate::arrays::VarBinViewArray). pub fn append_varbinview( &mut self, - array: crate::ArrayView<'_, VarBinView>, + array: ArrayView<'_, VarBinView>, ctx: &mut ExecutionCtx, ) -> VortexResult<()> { let validity = array .varbinview_validity() .execute_mask(array.as_ref().len(), ctx)?; - match &validity { - Mask::AllTrue(_) => { - for index in 0..array.as_ref().len() { - self.append_value(array.bytes_at(index)); - } - } - Mask::AllFalse(len) => self.push_nulls(*len), - Mask::Values(values) => { - for (index, is_valid) in values.bit_buffer().iter().enumerate() { - if is_valid { - self.append_value(array.bytes_at(index)); - } else { - self.push_null(); - } - } - } + match &mut self.storage { + DynOffsets::I32(builder) => append_varbinview_to(builder, array, &validity), + DynOffsets::I64(builder) => append_varbinview_to(builder, array, &validity), } Ok(()) } @@ -363,25 +376,57 @@ impl DynVarBinBuilder { } } - fn append_value(&mut self, value: impl AsRef<[u8]>) { + fn push_nulls(&mut self, n: usize) { match &mut self.storage { - DynOffsets::I32(builder) => builder.append_value(value), - DynOffsets::I64(builder) => builder.append_value(value), + DynOffsets::I32(builder) => builder.append_n_nulls(n), + DynOffsets::I64(builder) => builder.append_n_nulls(n), } } +} - fn push_null(&mut self) { - match &mut self.storage { - DynOffsets::I32(builder) => builder.append_null(), - DynOffsets::I64(builder) => builder.append_null(), +/// Appends `array` to a [`VarBinBuilder`] with a statically known offset type. +/// +/// [`DynVarBinBuilder`] resolves its offset width once before calling this, so the loops below +/// append through a monomorphized [`VarBinBuilder`] rather than re-reading [`DynOffsets`] per row. +fn append_varbinview_to( + builder: &mut VarBinBuilder, + array: ArrayView<'_, VarBinView>, + validity: &Mask, +) { + // Read the views slice once. `bytes_at` would re-derive it and hand back a refcounted + // `ByteBuffer` per row; `view_bytes` borrows the payload instead. + let views = array.views(); + match validity { + Mask::AllTrue(len) => { + builder.reserve_exact(*len); + for view in &views[..*len] { + builder.append_value(view_bytes(&array, view)); + } + } + Mask::AllFalse(len) => builder.append_n_nulls(*len), + Mask::Values(values) => { + builder.reserve_exact(values.len()); + // Offsets and validity must be appended in row order, so this cannot use the + // set-index fast paths: the null rows carry offsets too. + for (view, is_valid) in views[..values.len()].iter().zip(values.bit_buffer().iter()) { + if is_valid { + builder.append_value(view_bytes(&array, view)); + } else { + builder.append_null(); + } + } } } +} - fn push_nulls(&mut self, n: usize) { - match &mut self.storage { - DynOffsets::I32(builder) => builder.append_n_nulls(n), - DynOffsets::I64(builder) => builder.append_n_nulls(n), - } +/// Borrows the bytes of a single view without constructing a [`ByteBuffer`]. +#[inline] +fn view_bytes<'a>(array: &'a ArrayView<'_, VarBinView>, view: &'a BinaryView) -> &'a [u8] { + if view.is_inlined() { + view.as_inlined().value() + } else { + let view_ref = view.as_view(); + &array.buffer(view_ref.buffer_index as usize).as_slice()[view_ref.as_range()] } } @@ -406,8 +451,9 @@ impl ArrayBuilder for DynVarBinBuilder { } fn append_zeros(&mut self, n: usize) { - for _ in 0..n { - self.append_value([]); + match &mut self.storage { + DynOffsets::I32(builder) => builder.append_n_empty(n), + DynOffsets::I64(builder) => builder.append_n_empty(n), } } @@ -588,6 +634,55 @@ mod tests { Ok(()) } + #[rstest] + #[case(false)] + #[case(true)] + fn append_zeros_appends_non_null_empty_values(#[case] large_offsets: bool) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let mut builder = DynVarBinBuilder::with_capacity(DType::Utf8(Nullable), large_offsets, 0); + + builder.append_scalar(&Scalar::utf8("hello", Nullable))?; + builder.append_zeros(3); + + assert_eq!(builder.len(), 4); + let expected = VarBinArray::from_iter( + [Some("hello"), Some(""), Some(""), Some("")], + DType::Utf8(Nullable), + ); + assert_arrays_eq!(builder.finish_into_varbin(), expected, &mut ctx); + Ok(()) + } + + #[rstest] + #[case(false)] + #[case(true)] + fn append_n_values_repeats_the_value(#[case] large_offsets: bool) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let mut builder = DynVarBinBuilder::with_capacity(DType::Utf8(Nullable), large_offsets, 0); + + builder.append_n_values("ab", 2); + builder.append_null(); + builder.append_n_values("", 2); + + let expected = VarBinArray::from_iter( + [Some("ab"), Some("ab"), None, Some(""), Some("")], + DType::Utf8(Nullable), + ); + assert_arrays_eq!(builder.finish_into_varbin(), expected, &mut ctx); + Ok(()) + } + + #[test] + fn append_n_empty_preserves_previous_offset() { + let mut builder = VarBinBuilder::::new(); + builder.append_value(b"abc"); + builder.append_n_empty(2); + let array = builder.finish(DType::Utf8(Nullable)); + + assert_eq!(array.len(), 3); + assert_eq!(array.bytes().len(), 3); + } + #[test] fn offsets_have_is_sorted_stat() -> VortexResult<()> { let mut builder = VarBinBuilder::::with_capacity(0);