Skip to content

Commit 20dd597

Browse files
clauderobert3005
authored andcommitted
feat(array): let callers choose a nested builder's chunk threshold
`ChildBuilder` copies appended arrays shorter than 64 values instead of giving them a chunk, which is right for the nested builders that append the elements of a single list, and wrong for a caller whose appended arrays are themselves chunks worth preserving. Add `ArrayBuilder::set_min_chunk_len`, a no-op for builders without array children, so such a caller can lower the threshold — to zero to keep every chunk boundary. The threshold is read when an array is appended and applies transitively to the children of a builder's children. Signed-off-by: Claude <noreply@anthropic.com>
1 parent c78ed3c commit 20dd597

8 files changed

Lines changed: 135 additions & 5 deletions

File tree

vortex-array/src/builders/child.rs

Lines changed: 69 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,16 @@ use crate::dtype::Nullability;
1818
use crate::scalar::Scalar;
1919
use crate::validity::Validity;
2020

21-
/// The shortest array that a [`ChildBuilder`] keeps as a chunk of its own.
21+
/// The default shortest array that a [`ChildBuilder`] keeps as a chunk of its own.
2222
///
2323
/// Nested builders routinely append very short arrays (the elements of a single list, for
2424
/// example), and giving each one its own chunk would produce a [`ChunkedArray`] with more chunks
2525
/// than values. Below this length, copying the values costs less than the indirection that every
2626
/// later access to the chunk would pay.
27-
pub(super) const MIN_CHUNK_LEN: usize = 64;
27+
///
28+
/// Callers that would rather keep every chunk boundary, however short, can lower the threshold
29+
/// with [`ArrayBuilder::set_min_chunk_len`].
30+
pub(super) const DEFAULT_MIN_CHUNK_LEN: usize = 64;
2831

2932
/// Accumulates the child of a nested [`ArrayBuilder`] without canonicalizing appended arrays.
3033
///
@@ -51,6 +54,9 @@ pub struct ChildBuilder {
5154

5255
/// Builder holding the scalars appended after the last chunk.
5356
pending: Box<dyn ArrayBuilder>,
57+
58+
/// The shortest appended array that is kept as a chunk rather than copied.
59+
min_chunk_len: usize,
5460
}
5561

5662
impl ChildBuilder {
@@ -61,9 +67,18 @@ impl ChildBuilder {
6167
chunks: Vec::new(),
6268
chunks_len: 0,
6369
pending: builder_with_capacity(dtype, capacity),
70+
min_chunk_len: DEFAULT_MIN_CHUNK_LEN,
6471
}
6572
}
6673

74+
/// Sets the shortest appended array that is kept as a chunk rather than copied.
75+
///
76+
/// See [`ArrayBuilder::set_min_chunk_len`].
77+
pub fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
78+
self.min_chunk_len = min_chunk_len;
79+
self.pending.set_min_chunk_len(min_chunk_len);
80+
}
81+
6782
/// The number of values appended so far.
6883
pub fn len(&self) -> usize {
6984
self.chunks_len + self.pending.len()
@@ -85,7 +100,7 @@ impl ChildBuilder {
85100
return Ok(());
86101
}
87102

88-
if array.len() < MIN_CHUNK_LEN {
103+
if array.len() < self.min_chunk_len {
89104
return array.append_to_builder(self.pending.as_mut(), ctx);
90105
}
91106

@@ -196,14 +211,16 @@ impl ChildBuilder {
196211

197212
#[cfg(test)]
198213
mod tests {
214+
use std::sync::Arc;
215+
199216
use rstest::rstest;
200217
use vortex_buffer::buffer;
201218
use vortex_error::VortexExpect;
202219
use vortex_error::VortexResult;
203220
use vortex_mask::Mask;
204221

205222
use super::ChildBuilder;
206-
use super::MIN_CHUNK_LEN;
223+
use super::DEFAULT_MIN_CHUNK_LEN as MIN_CHUNK_LEN;
207224
use crate::ArrayRef;
208225
use crate::IntoArray;
209226
use crate::VortexSessionExecute;
@@ -212,16 +229,20 @@ mod tests {
212229
use crate::arrays::ChunkedArray;
213230
use crate::arrays::Constant;
214231
use crate::arrays::ConstantArray;
232+
use crate::arrays::ListView;
233+
use crate::arrays::ListViewArray;
215234
use crate::arrays::Masked;
216235
use crate::arrays::Primitive;
217236
use crate::arrays::PrimitiveArray;
218237
use crate::arrays::chunked::ChunkedArrayExt;
238+
use crate::arrays::listview::ListViewArraySlotsExt;
219239
use crate::assert_arrays_eq;
220240
use crate::dtype::DType;
221241
use crate::dtype::Nullability::NonNullable;
222242
use crate::dtype::Nullability::Nullable;
223243
use crate::dtype::PType::I32;
224244
use crate::scalar::Scalar;
245+
use crate::validity::Validity;
225246

226247
/// A non-canonical array of `len` values, all equal to `value`.
227248
fn constant(value: i32, len: usize) -> ArrayRef {
@@ -476,6 +497,50 @@ mod tests {
476497
unsafe { builder.set_validity_unchecked(Mask::new_true(MIN_CHUNK_LEN)) };
477498
}
478499

500+
/// A threshold of zero keeps every chunk boundary, however short.
501+
#[test]
502+
fn test_min_chunk_len_zero_keeps_every_chunk() -> VortexResult<()> {
503+
let mut ctx = array_session().create_execution_ctx();
504+
let mut builder = ChildBuilder::with_capacity(&DType::from(I32), 0);
505+
builder.set_min_chunk_len(0);
506+
507+
builder.append_array(&constant(1, 1), &mut ctx)?;
508+
builder.append_array(&constant(2, 1), &mut ctx)?;
509+
510+
let child = builder.finish();
511+
let chunked = child.as_::<Chunked>();
512+
assert_eq!(chunked.nchunks(), 2);
513+
assert!(chunked.iter_chunks().all(|chunk| chunk.is::<Constant>()));
514+
515+
Ok(())
516+
}
517+
518+
/// The threshold reaches the children of a child: an array short enough to be materialized
519+
/// lands in the scalar builder, which is itself a nested builder with children of its own.
520+
#[test]
521+
fn test_min_chunk_len_applies_transitively() -> VortexResult<()> {
522+
let mut ctx = array_session().create_execution_ctx();
523+
let dtype = DType::List(Arc::new(DType::from(I32)), NonNullable);
524+
let mut builder = ChildBuilder::with_capacity(&dtype, 0);
525+
builder.set_min_chunk_len(4 * MIN_CHUNK_LEN);
526+
527+
// Two lists — far too few to be a chunk — holding enough elements between them that those
528+
// elements would become a chunk if the threshold stopped at the outer builder.
529+
let lists = ListViewArray::new(
530+
constant(1, 2 * MIN_CHUNK_LEN),
531+
buffer![0u64, MIN_CHUNK_LEN as u64].into_array(),
532+
buffer![MIN_CHUNK_LEN as u64; 2].into_array(),
533+
Validity::NonNullable,
534+
)
535+
.into_array();
536+
builder.append_array(&lists, &mut ctx)?;
537+
538+
let child = builder.finish();
539+
assert!(child.as_::<ListView>().elements().is::<Primitive>());
540+
541+
Ok(())
542+
}
543+
479544
#[test]
480545
fn test_finish_resets_the_builder() -> VortexResult<()> {
481546
let mut ctx = array_session().create_execution_ctx();

vortex-array/src/builders/extension.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,10 @@ impl ArrayBuilder for ExtensionBuilder {
110110
self.append_value(scalar.as_extension())
111111
}
112112

113+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
114+
self.storage.set_min_chunk_len(min_chunk_len);
115+
}
116+
113117
fn reserve_exact(&mut self, capacity: usize) {
114118
self.storage.reserve_exact(capacity)
115119
}

vortex-array/src/builders/fixed_size_list.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,10 @@ impl ArrayBuilder for FixedSizeListBuilder {
255255

256256
/// This will increase the capacity if extending with this `array` would go past the original
257257
/// capacity.
258+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
259+
self.elements_builder.set_min_chunk_len(min_chunk_len);
260+
}
261+
258262
fn reserve_exact(&mut self, additional: usize) {
259263
self.elements_builder
260264
.reserve_exact(additional * self.list_size() as usize);

vortex-array/src/builders/list.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,10 @@ impl<O: IntegerPType> ArrayBuilder for ListBuilder<O> {
274274
self.append_value(scalar.as_list())
275275
}
276276

277+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
278+
self.elements_builder.set_min_chunk_len(min_chunk_len);
279+
}
280+
277281
fn reserve_exact(&mut self, additional: usize) {
278282
self.elements_builder.reserve_exact(additional);
279283
self.offsets_builder.reserve_exact(additional);

vortex-array/src/builders/listview.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,10 @@ impl<O: IntegerPType, S: IntegerPType> ArrayBuilder for ListViewBuilder<O, S> {
311311
self.append_value(list_scalar)
312312
}
313313

314+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
315+
self.elements_builder.set_min_chunk_len(min_chunk_len);
316+
}
317+
314318
fn reserve_exact(&mut self, capacity: usize) {
315319
self.elements_builder.reserve_exact(capacity * 2);
316320
self.offsets_builder.reserve_exact(capacity);

vortex-array/src/builders/mod.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,20 @@ pub trait ArrayBuilder: Send {
170170
/// Allocate space for extra `additional` items
171171
fn reserve_exact(&mut self, additional: usize);
172172

173+
/// Sets the shortest array that this builder's children keep as a chunk of their own.
174+
///
175+
/// Arrays shorter than this are copied into a canonical child instead, which costs a copy but
176+
/// spares every later reader the chunk indirection. Pass `0` to keep every chunk boundary,
177+
/// however short — worthwhile when the appended arrays are themselves chunks whose identity is
178+
/// the point, as when canonicalizing a [`ChunkedArray`](crate::arrays::ChunkedArray).
179+
///
180+
/// The threshold is read when an array is appended, so it only affects subsequent appends. It
181+
/// applies transitively to the children of this builder's children. Builders without array
182+
/// children ignore it.
183+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
184+
let _ = min_chunk_len;
185+
}
186+
173187
/// Override builders validity with the one provided.
174188
///
175189
/// Note that this will have no effect on the final array if the array builder is non-nullable.

vortex-array/src/builders/struct_.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,12 @@ impl ArrayBuilder for StructBuilder {
184184
self.append_value(scalar.as_struct())
185185
}
186186

187+
fn set_min_chunk_len(&mut self, min_chunk_len: usize) {
188+
for builder in &mut self.builders {
189+
builder.set_min_chunk_len(min_chunk_len);
190+
}
191+
}
192+
187193
fn reserve_exact(&mut self, capacity: usize) {
188194
self.builders.iter_mut().for_each(|builder| {
189195
builder.reserve_exact(capacity);

vortex-array/src/builders/tests.rs

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ use crate::assert_arrays_eq;
4343
use crate::builders::ArrayBuilder;
4444
use crate::builders::ListBuilder;
4545
use crate::builders::builder_with_capacity;
46-
use crate::builders::child::MIN_CHUNK_LEN;
46+
use crate::builders::child::DEFAULT_MIN_CHUNK_LEN as MIN_CHUNK_LEN;
4747
use crate::dtype::DType;
4848
use crate::dtype::DecimalDType;
4949
use crate::dtype::Nullability;
@@ -1053,6 +1053,35 @@ fn test_struct_builder_interleaves_arrays_and_scalars() -> VortexResult<()> {
10531053
Ok(())
10541054
}
10551055

1056+
/// Lowering the threshold to zero keeps every chunk boundary a nested builder is handed, however
1057+
/// short — what a caller wants when the appended arrays are themselves chunks worth preserving.
1058+
#[test]
1059+
fn test_min_chunk_len_zero_preserves_short_chunks() -> VortexResult<()> {
1060+
let mut ctx = array_session().create_execution_ctx();
1061+
1062+
let array = StructArray::try_from_iter([("a", buffer![1i32])])?.into_array();
1063+
let mut builder = builder_with_capacity(array.dtype(), 0);
1064+
builder.set_min_chunk_len(0);
1065+
array.append_to_builder(builder.as_mut(), &mut ctx)?;
1066+
array.append_to_builder(builder.as_mut(), &mut ctx)?;
1067+
let built = builder.finish();
1068+
1069+
assert_eq!(
1070+
built
1071+
.as_::<Struct>()
1072+
.unmasked_field(0)
1073+
.as_::<Chunked>()
1074+
.nchunks(),
1075+
2,
1076+
"a one-row field should still have earned a chunk",
1077+
);
1078+
1079+
let expected = ChunkedArray::try_new(vec![array.clone(), array], built.dtype().clone())?;
1080+
assert_arrays_eq!(&built, &expected, &mut ctx);
1081+
1082+
Ok(())
1083+
}
1084+
10561085
/// A non-canonical array of [`MIN_CHUNK_LEN`] `i32` values.
10571086
fn constant_i32() -> ArrayRef {
10581087
ConstantArray::new(0i32, MIN_CHUNK_LEN).into_array()

0 commit comments

Comments
 (0)