Skip to content

Commit 0e6e08d

Browse files
committed
VarBin: build views straight into a VarBinViewBuilder
Appending a `VarBin` array to a `VarBinViewBuilder` canonicalized it first, which built the views, wrapped them in a `VarBinViewArray` the builder immediately unwrapped, and then had `append_varbinview_array` rewrite every view to rebase its buffer index onto the builder's. Number the buffer up front instead: `varbin_decode_views` takes the index the pushed buffer will land at, so the views come out already correct and the append is one view per row plus a buffer push. `varbin_to_canonical` now shares that helper, which also shrinks what `match_each_integer_ptype!` duplicates. A builder configured to compact still goes the canonical route — it chooses per buffer whether to keep, slice or rewrite it by measuring the finished views, and `push_buffer_and_adjusted_views` would bypass that. chunk_array_builder, fastest of 100 samples, 3 runs each: | benchmark (rows x chunks) | before | after | speedup | | ------------------------------------- | ------- | ------- | ------- | | varbin_to_varbinview_builder 10x1000 | 313 µs | 204 µs | 1.53x | | varbin_to_varbinview_builder 100x100 | 87.3 µs | 69.5 µs | 1.26x | | varbin_to_varbinview_builder 1000x10 | 58.6 µs | 54.8 µs | 1.07x | | varbin_opt_to_varbinview_bldr 10x1000 | 378 µs | 266 µs | 1.42x | | varbin_into_canonical 10x1000 | 357 µs | 253 µs | 1.41x | The last row is the control: it never touches the builder, so its gain is from the shared-helper extraction alone. Relative to it, the append itself drops from 0.88x to 0.81x of a canonicalization. Signed-off-by: Robert Kruszewski <github@robertk.io>
1 parent 6d11fc1 commit 0e6e08d

4 files changed

Lines changed: 240 additions & 11 deletions

File tree

vortex-array/benches/chunk_array_builder.rs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use vortex_array::arrays::BoolArray;
1717
use vortex_array::arrays::ChunkedArray;
1818
use vortex_array::arrays::ConstantArray;
1919
use vortex_array::builders::ArrayBuilder;
20+
use vortex_array::builders::VarBinBuilder;
2021
use vortex_array::builders::VarBinViewBuilder;
2122
use vortex_array::builders::builder_with_capacity;
2223
use vortex_array::dtype::DType;
@@ -133,6 +134,52 @@ fn chunked_varbinview_opt_into_canonical(bencher: Bencher, (len, chunk_count): (
133134
.bench_refs(|(chunk, ctx)| (**chunk).clone().execute::<Canonical>(ctx))
134135
}
135136

137+
/// `VarBin` chunks into a `VarBinViewBuilder`: the views are built straight over each chunk's
138+
/// value bytes and numbered against the builder's buffers, rather than canonicalizing each chunk
139+
/// into a `VarBinViewArray` whose views then get rewritten to rebase their buffer index.
140+
#[divan::bench(args = BENCH_ARGS)]
141+
fn chunked_varbin_to_varbinview_builder(bencher: Bencher, (len, chunk_count): (usize, usize)) {
142+
let chunks = make_varbin_chunks(false, len, chunk_count);
143+
144+
bencher
145+
.with_inputs(|| (&chunks, SESSION.create_execution_ctx()))
146+
.bench_refs(|(chunk, ctx)| {
147+
let mut builder =
148+
VarBinViewBuilder::with_capacity(chunk.dtype().clone(), len * chunk_count);
149+
chunk
150+
.append_to_builder(&mut builder, ctx)
151+
.vortex_expect("append failed");
152+
builder.finish()
153+
})
154+
}
155+
156+
#[divan::bench(args = BENCH_ARGS)]
157+
fn chunked_varbin_opt_to_varbinview_builder(bencher: Bencher, (len, chunk_count): (usize, usize)) {
158+
let chunks = make_varbin_chunks(true, len, chunk_count);
159+
160+
bencher
161+
.with_inputs(|| (&chunks, SESSION.create_execution_ctx()))
162+
.bench_refs(|(chunk, ctx)| {
163+
let mut builder =
164+
VarBinViewBuilder::with_capacity(chunk.dtype().clone(), len * chunk_count);
165+
chunk
166+
.append_to_builder(&mut builder, ctx)
167+
.vortex_expect("append failed");
168+
builder.finish()
169+
})
170+
}
171+
172+
/// The same chunks routed through `execute::<Canonical>`, which is what the builder append used to
173+
/// do per chunk before pushing the result.
174+
#[divan::bench(args = BENCH_ARGS)]
175+
fn chunked_varbin_into_canonical(bencher: Bencher, (len, chunk_count): (usize, usize)) {
176+
let chunks = make_varbin_chunks(false, len, chunk_count);
177+
178+
bencher
179+
.with_inputs(|| (&chunks, SESSION.create_execution_ctx()))
180+
.bench_refs(|(chunk, ctx)| (**chunk).clone().execute::<Canonical>(ctx))
181+
}
182+
136183
#[divan::bench(args = BENCH_ARGS)]
137184
fn chunked_constant_i32_append_to_builder(bencher: Bencher, (len, chunk_count): (usize, usize)) {
138185
let chunk = make_constant_i32_chunks(len, chunk_count);
@@ -226,6 +273,32 @@ fn make_bool_chunks(len: usize, chunk_count: usize) -> ArrayRef {
226273
.into_array()
227274
}
228275

276+
/// Chunks of `VarBin`, with a mix of inlinable (≤12 byte) and buffer-referencing values so both
277+
/// halves of the view construction are exercised.
278+
fn make_varbin_chunks(nullable: bool, len: usize, chunk_count: usize) -> ArrayRef {
279+
let mut rng = StdRng::seed_from_u64(123);
280+
let dtype = DType::Utf8(nullable.into());
281+
282+
(0..chunk_count)
283+
.map(|_| {
284+
let mut builder = VarBinBuilder::<i32>::with_capacity(dtype.clone(), len);
285+
(0..len).for_each(|_| {
286+
if nullable && rng.random_bool(0.2) {
287+
builder.push_null()
288+
} else {
289+
builder.append_value(
290+
(0..rng.random_range(0..=20))
291+
.map(|_| rng.random_range(b'a'..=b'z'))
292+
.collect::<Vec<u8>>(),
293+
)
294+
}
295+
});
296+
builder.finish()
297+
})
298+
.collect::<ChunkedArray>()
299+
.into_array()
300+
}
301+
229302
fn make_string_chunks(nullable: bool, len: usize, chunk_count: usize) -> ArrayRef {
230303
let mut rng = StdRng::seed_from_u64(123);
231304

vortex-array/src/arrays/varbin/vtable/canonical.rs

Lines changed: 105 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,20 @@
44
use std::sync::Arc;
55

66
use num_traits::AsPrimitive;
7+
use vortex_buffer::Buffer;
8+
use vortex_buffer::ByteBuffer;
79
use vortex_error::VortexResult;
810

911
use crate::ExecutionCtx;
1012
use crate::array::ArrayView;
1113
use crate::arrays::PrimitiveArray;
1214
use crate::arrays::VarBin;
1315
use crate::arrays::VarBinViewArray;
16+
use crate::arrays::varbinview::BinaryView;
1417
use crate::arrays::varbinview::build_views::MAX_BUFFER_LEN;
1518
use crate::arrays::varbinview::build_views::build_views;
1619
use crate::arrays::varbinview::build_views::offsets_to_lengths;
20+
use crate::buffer::BufferHandle;
1721
use crate::match_each_integer_ptype;
1822

1923
/// Converts a VarBinArray to its canonical form (VarBinViewArray).
@@ -24,35 +28,54 @@ pub(crate) fn varbin_to_canonical(
2428
ctx: &mut ExecutionCtx,
2529
) -> VortexResult<VarBinViewArray> {
2630
let parts = array.into_owned().into_data_parts();
27-
2831
let offsets = parts.offsets.execute::<PrimitiveArray>(ctx)?;
32+
let (buffers, views) = varbin_decode_views(&offsets, parts.bytes, 0);
33+
34+
// SAFETY: views are correctly computed from valid offsets
35+
Ok(unsafe {
36+
VarBinViewArray::new_unchecked(views, Arc::from(buffers), parts.dtype, parts.validity)
37+
})
38+
}
2939

40+
/// Lays a `VarBin` array's value bytes out as `VarBinView` buffers plus the views over them.
41+
///
42+
/// `start_buf_index` is the index the first returned buffer will occupy in its destination, so the
43+
/// views come out already referencing the right buffer and never need rebasing. Canonicalization
44+
/// passes `0`; appending into a [`VarBinViewBuilder`](crate::builders::VarBinViewBuilder) passes the
45+
/// index its next buffer will land at.
46+
///
47+
/// The value bytes are handed over as they are — only the offsets are consumed, to derive the view
48+
/// lengths — so this costs one view per row and no byte copy when the buffer is uniquely held.
49+
pub(crate) fn varbin_decode_views(
50+
offsets: &PrimitiveArray,
51+
bytes: BufferHandle,
52+
start_buf_index: u32,
53+
) -> (Vec<ByteBuffer>, Buffer<BinaryView>) {
3054
match_each_integer_ptype!(offsets.ptype(), |P| {
3155
let offsets_slice = offsets.as_slice::<P>();
3256
let first: usize = offsets_slice[0].as_();
3357
let last: usize = offsets_slice[offsets_slice.len() - 1].as_();
34-
let bytes = parts.bytes.unwrap_host().slice(first..last).into_mut();
58+
let bytes = bytes.unwrap_host().slice(first..last).into_mut();
3559

3660
let lens = offsets_to_lengths(offsets_slice);
37-
let (buffers, views) = build_views(0, MAX_BUFFER_LEN, bytes, lens.as_slice());
38-
39-
// SAFETY: views are correctly computed from valid offsets
40-
Ok(unsafe {
41-
VarBinViewArray::new_unchecked(views, Arc::from(buffers), parts.dtype, parts.validity)
42-
})
61+
build_views(start_buf_index, MAX_BUFFER_LEN, bytes, lens.as_slice())
4362
})
4463
}
4564

4665
#[cfg(test)]
4766
mod tests {
4867
use rstest::rstest;
68+
use vortex_error::VortexResult;
4969

70+
use crate::IntoArray;
5071
use crate::VortexSessionExecute;
5172
use crate::array_session;
73+
use crate::arrays::ChunkedArray;
5274
use crate::arrays::VarBinArray;
5375
use crate::arrays::VarBinViewArray;
5476
use crate::arrays::varbin::builder::VarBinBuilder;
5577
use crate::assert_arrays_eq;
78+
use crate::builders::VarBinViewBuilder;
5679
use crate::dtype::DType;
5780
use crate::dtype::Nullability;
5881

@@ -108,6 +131,80 @@ mod tests {
108131
assert_arrays_eq!(canonical, expected, &mut ctx);
109132
}
110133

134+
/// Appending a `VarBin` array to a `VarBinViewBuilder` builds views over its bytes directly
135+
/// instead of canonicalizing first, so the views must be numbered against the buffers the
136+
/// builder already holds. Interleaving `VarBin` appends with value appends (which stage an
137+
/// in-progress buffer) and with a `VarBinView` append exercises that numbering.
138+
#[rstest]
139+
#[case(DType::Utf8(Nullability::Nullable))]
140+
#[case(DType::Binary(Nullability::Nullable))]
141+
fn append_varbin_to_varbinview_builder(#[case] dtype: DType) -> VortexResult<()> {
142+
let mut ctx = array_session().create_execution_ctx();
143+
let long = "a value long enough that its view has to reference a buffer";
144+
let longer = "another value long enough that its view has to reference a buffer";
145+
146+
// Two chunks, each with an inlined value, a buffer-referencing value and a null, so both
147+
// pushed buffers are non-empty and the second must not reuse the first's index.
148+
let first = VarBinArray::from_iter([Some("short"), None, Some(long)], dtype.clone());
149+
let second = VarBinArray::from_iter([Some(longer), Some("tiny"), None], dtype.clone());
150+
let view = VarBinViewArray::from_iter([Some(long), None], dtype.clone());
151+
152+
let mut builder = VarBinViewBuilder::with_capacity(dtype.clone(), 8);
153+
first
154+
.as_array()
155+
.clone()
156+
.append_to_builder(&mut builder, &mut ctx)?;
157+
// Stages an in-progress buffer, which the next append has to account for.
158+
builder.append_value(longer);
159+
second
160+
.as_array()
161+
.clone()
162+
.append_to_builder(&mut builder, &mut ctx)?;
163+
view.clone()
164+
.into_array()
165+
.append_to_builder(&mut builder, &mut ctx)?;
166+
167+
let expected = ChunkedArray::try_new(
168+
vec![
169+
first.as_array().clone(),
170+
VarBinViewArray::from_iter([Some(longer)], dtype.clone()).into_array(),
171+
second.as_array().clone(),
172+
view.into_array(),
173+
],
174+
dtype,
175+
)?;
176+
assert_arrays_eq!(builder.finish_into_varbinview(), expected, &mut ctx);
177+
Ok(())
178+
}
179+
180+
/// A builder configured to compact must not be handed a raw buffer behind its back: the
181+
/// inlined values leave the pushed buffer only partly referenced, and skipping compaction
182+
/// would keep those bytes alive. Appending through the canonical array instead drops them.
183+
#[test]
184+
fn append_varbin_to_a_compacting_builder_still_compacts() -> VortexResult<()> {
185+
let mut ctx = array_session().create_execution_ctx();
186+
let dtype = DType::Utf8(Nullability::NonNullable);
187+
// Every value inlines, so nothing references the value bytes at all.
188+
let array = VarBinArray::from_iter_nonnull(["short", "tiny", "small"], dtype.clone());
189+
190+
let mut builder = VarBinViewBuilder::with_compaction(dtype, 4, 1.0);
191+
array
192+
.as_array()
193+
.clone()
194+
.append_to_builder(&mut builder, &mut ctx)?;
195+
let compacted = builder.finish_into_varbinview();
196+
197+
assert!(
198+
compacted
199+
.data_buffers()
200+
.iter()
201+
.all(|buffer| buffer.is_empty()),
202+
"a fully-inlined append should not retain any value bytes"
203+
);
204+
assert_arrays_eq!(compacted, array.as_array().clone(), &mut ctx);
205+
Ok(())
206+
}
207+
111208
// Empty array: offsets has exactly one element; no elements to canonicalize.
112209
#[test]
113210
fn test_canonical_varbin_empty() {

vortex-array/src/arrays/varbin/vtable/mod.rs

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,14 @@ use crate::array::Array;
2020
use crate::array::ArrayId;
2121
use crate::array::ArrayView;
2222
use crate::array::VTable;
23+
use crate::arrays::PrimitiveArray;
24+
use crate::arrays::varbin::VarBinArrayExt;
2325
use crate::arrays::varbin::VarBinArraySlotsExt;
2426
use crate::arrays::varbin::VarBinData;
2527
use crate::arrays::varbin::VarBinSlots;
2628
use crate::buffer::BufferHandle;
2729
use crate::builders::ArrayBuilder;
30+
use crate::builders::VarBinViewBuilder;
2831
use crate::dtype::DType;
2932
use crate::dtype::Nullability;
3033
use crate::dtype::PType;
@@ -36,6 +39,7 @@ mod kernel;
3639
mod operations;
3740
mod validity;
3841

42+
use canonical::varbin_decode_views;
3943
use canonical::varbin_to_canonical;
4044
use vortex_session::VortexSession;
4145

@@ -214,9 +218,27 @@ impl VTable for VarBin {
214218
{
215219
return result;
216220
}
217-
varbin_to_canonical(array, ctx)?
218-
.into_array()
219-
.append_to_builder(builder, ctx)
221+
222+
// The two arms here are every builder a `Utf8`/`Binary` dtype has: all four
223+
// `VarBinBuilder` widths above, and `VarBinViewBuilder` below.
224+
let Some(view_builder) = builder.as_any().downcast_ref::<VarBinViewBuilder>() else {
225+
vortex_bail!("append_to_builder for VarBin requires a variable-binary builder")
226+
};
227+
228+
if view_builder.compacts_buffers() {
229+
// A compacting builder decides per buffer whether to keep, slice or rewrite it, which
230+
// it can only do by measuring the finished views against the buffer. Go through the
231+
// canonical array so that policy still applies.
232+
return varbin_to_canonical(array, ctx)?
233+
.into_array()
234+
.append_to_builder(builder, ctx);
235+
}
236+
237+
let builder = builder
238+
.as_any_mut()
239+
.downcast_mut::<VarBinViewBuilder>()
240+
.vortex_expect("builder type checked above");
241+
append_to_varbinview(array, builder, ctx)
220242
}
221243

222244
fn execute(array: Array<Self>, ctx: &mut ExecutionCtx) -> VortexResult<ExecutionResult> {
@@ -226,5 +248,32 @@ impl VTable for VarBin {
226248
}
227249
}
228250

251+
/// Hands the value bytes to `builder` as a data buffer with views built over them.
252+
///
253+
/// Canonicalizing first would build the same views, then pay for them twice more: once to wrap
254+
/// them in a `VarBinViewArray` the builder immediately unwraps, and once for
255+
/// `append_varbinview_array` to rewrite every view so its buffer index is rebased onto the
256+
/// builder's. Numbering the buffer up front instead makes the whole append one view per row plus
257+
/// pushing the byte buffer.
258+
fn append_to_varbinview(
259+
array: ArrayView<'_, VarBin>,
260+
builder: &mut VarBinViewBuilder,
261+
ctx: &mut ExecutionCtx,
262+
) -> VortexResult<()> {
263+
let len = array.as_ref().len();
264+
let validity = array.varbin_validity().execute_mask(len, ctx)?;
265+
266+
// Build the views against the index the pushed buffer will land at, so the builder does not
267+
// have to rebase them afterwards.
268+
let next_buffer_index = builder.completed_block_count() + u32::from(builder.in_progress());
269+
270+
let parts = array.into_owned().into_data_parts();
271+
let offsets = parts.offsets.execute::<PrimitiveArray>(ctx)?;
272+
let (buffers, views) = varbin_decode_views(&offsets, parts.bytes, next_buffer_index);
273+
274+
builder.push_buffer_and_adjusted_views(&buffers, &views, validity);
275+
Ok(())
276+
}
277+
229278
#[derive(Clone, Debug)]
230279
pub struct VarBin;

vortex-array/src/builders/varbinview.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,16 @@ impl VarBinViewBuilder {
194194
self.in_progress.is_some()
195195
}
196196

197+
/// Whether this builder compacts the data buffers it is handed.
198+
///
199+
/// [`push_buffer_and_adjusted_views`](Self::push_buffer_and_adjusted_views) takes buffers
200+
/// exactly as they are, so an encoding that would push a buffer only partly covered by its
201+
/// views should check this first and fall back to a route that measures utilization —
202+
/// otherwise it silently opts the builder out of the compaction it was configured for.
203+
pub fn compacts_buffers(&self) -> bool {
204+
self.compaction_threshold > 0.0
205+
}
206+
197207
/// Pushes buffers and pre-adjusted views into the builder.
198208
///
199209
/// The provided `buffers` contain sections of data from a `VarBinViewArray`, and the

0 commit comments

Comments
 (0)