Skip to content

Commit b622d0f

Browse files
robert3005claude
andcommitted
perf(array): append repeated and viewed lists in bulk, not a value at a time
Three callers walked a builder one list at a time where the whole run was available up front. `Sparse` canonicalization fills the gaps between patches by appending the fill value once per row. A gap is a run of one value, so it goes in as a single `ConstantArray` instead: canonicalizing a constant list array points every view at one copy of the value, so a gap now costs the fill value's elements once however many rows it covers. Appending a 10,000-row gap of a three-element fill produced 30,000 elements; it now produces 3. `ListBuilder::append_listview_array` sliced the elements array and appended the slice once per list, which is what its `ListViewBuilder` twin stopped doing. `ListArray` offsets can only describe contiguous, in-order lists, so flatten the incoming views to that layout - a no-op when they are laid out that way already - and then append the referenced elements in one go, walking only the metadata to rebase the offsets. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0779ba5 commit b622d0f

2 files changed

Lines changed: 179 additions & 93 deletions

File tree

encodings/sparse/src/canonical.rs

Lines changed: 87 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ use vortex_array::builders::ArrayBuilder;
3131
use vortex_array::builders::DecimalBuilder;
3232
use vortex_array::builders::FixedSizeListBuilder;
3333
use vortex_array::builders::ListViewBuilder;
34-
use vortex_array::builders::builder_with_capacity;
3534
use vortex_array::dtype::DType;
3635
use vortex_array::dtype::DecimalDType;
3736
use vortex_array::dtype::DecimalType;
@@ -47,7 +46,6 @@ use vortex_array::match_each_unsigned_integer_ptype;
4746
use vortex_array::match_smallest_offset_type;
4847
use vortex_array::patches::Patches;
4948
use vortex_array::scalar::DecimalScalar;
50-
use vortex_array::scalar::ListScalar;
5149
use vortex_array::scalar::Scalar;
5250
use vortex_array::scalar::StructScalar;
5351
use vortex_array::validity::Validity;
@@ -181,15 +179,19 @@ fn execute_sparse_lists(
181179
let values = resolved.values().as_::<ListView>().into_owned();
182180
let fill_list = fill_value.as_list();
183181

182+
// Each gap between patches is appended as one constant array, whose canonical form points
183+
// every view at a single copy of the fill value, so a gap costs the fill value's elements once
184+
// however many rows it covers. There is at most one gap on either side of each patch.
184185
let n_filled = len - resolved.num_patches();
185-
let total_canonical_values = values.elements().len() + fill_list.len() * n_filled;
186+
let n_fill_runs = (resolved.num_patches() + 1).min(n_filled);
187+
let total_canonical_values = values.elements().len() + fill_list.len() * n_fill_runs;
186188

187189
Ok(match_each_unsigned_integer_ptype!(indices.ptype(), |I| {
188190
match_smallest_offset_type!(total_canonical_values, |O| {
189191
execute_sparse_lists_inner::<I, O>(
190192
indices.as_slice(),
191193
values,
192-
fill_list,
194+
fill_value,
193195
values_dtype,
194196
len,
195197
total_canonical_values,
@@ -204,7 +206,7 @@ fn execute_sparse_lists(
204206
fn execute_sparse_lists_inner<I: IntegerPType, O: IntegerPType>(
205207
patch_indices: &[I],
206208
patch_values: ListViewArray,
207-
fill_scalar: ListScalar,
209+
fill_value: &Scalar,
208210
values_dtype: Arc<DType>,
209211
len: usize,
210212
total_canonical_values: usize,
@@ -219,7 +221,6 @@ fn execute_sparse_lists_inner<I: IntegerPType, O: IntegerPType>(
219221
total_canonical_values,
220222
len,
221223
);
222-
let fill_elements = list_scalar_elements_array(fill_scalar);
223224
let patch_values_validity = patch_values
224225
.listview_validity()
225226
.execute_mask(patch_values.len(), ctx)
@@ -236,12 +237,7 @@ fn execute_sparse_lists_inner<I: IntegerPType, O: IntegerPType>(
236237
.to_usize()
237238
.vortex_expect("patch index must fit in usize");
238239

239-
append_list_fill(
240-
&mut builder,
241-
fill_elements.as_ref(),
242-
sparse_idx - next_index,
243-
ctx,
244-
);
240+
append_fill(&mut builder, fill_value, sparse_idx - next_index, ctx);
245241

246242
if patch_valid {
247243
let patch_list = patch_values
@@ -257,7 +253,7 @@ fn execute_sparse_lists_inner<I: IntegerPType, O: IntegerPType>(
257253
next_index = sparse_idx + 1;
258254
}
259255

260-
append_list_fill(&mut builder, fill_elements.as_ref(), len - next_index, ctx);
256+
append_fill(&mut builder, fill_value, len - next_index, ctx);
261257

262258
builder.finish()
263259
}
@@ -272,13 +268,12 @@ fn execute_sparse_fixed_size_list(
272268
) -> VortexResult<ArrayRef> {
273269
let indices = resolved.indices().as_::<Primitive>().into_owned();
274270
let values = resolved.values().as_::<FixedSizeList>().into_owned();
275-
let fill_scalar = fill_value.as_list();
276271

277272
Ok(match_each_integer_ptype!(indices.ptype(), |I| {
278273
execute_sparse_fixed_size_list_inner::<I>(
279274
indices.as_slice(),
280275
values,
281-
fill_scalar,
276+
fill_value,
282277
len,
283278
nullability,
284279
ctx,
@@ -296,7 +291,7 @@ fn execute_sparse_fixed_size_list(
296291
fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
297292
indices: &[I],
298293
values: FixedSizeListArray,
299-
fill_scalar: ListScalar,
294+
fill_value: &Scalar,
300295
array_len: usize,
301296
nullability: Nullability,
302297
ctx: &mut ExecutionCtx,
@@ -312,7 +307,6 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
312307
nullability,
313308
array_len,
314309
);
315-
let fill_elements = list_scalar_elements_array(fill_scalar);
316310
let values_validity = values
317311
.validity()
318312
.vortex_expect("sparse fixed-size-list validity should be derivable")
@@ -326,12 +320,7 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
326320

327321
for ((patch_idx, sparse_idx), patch_valid) in indices.enumerate().zip(values_validity.iter()) {
328322
// Fill gap before this patch with fill values.
329-
append_fixed_size_list_fill(
330-
&mut builder,
331-
fill_elements.as_ref(),
332-
sparse_idx - next_index,
333-
ctx,
334-
);
323+
append_fill(&mut builder, fill_value, sparse_idx - next_index, ctx);
335324

336325
// Append the patch value, handling null patches by appending defaults.
337326
if patch_valid {
@@ -349,60 +338,38 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
349338
}
350339

351340
// Fill remaining positions after last patch.
352-
append_fixed_size_list_fill(
353-
&mut builder,
354-
fill_elements.as_ref(),
355-
array_len - next_index,
356-
ctx,
357-
);
341+
append_fill(&mut builder, fill_value, array_len - next_index, ctx);
358342

359343
builder.finish_into_fixed_size_list()
360344
}
361345

362-
fn list_scalar_elements_array(list: ListScalar) -> Option<ArrayRef> {
363-
list.elements().map(|elements| {
364-
let mut builder = builder_with_capacity(list.element_dtype(), elements.len());
365-
for element in elements {
366-
builder
367-
.append_scalar(&element)
368-
.vortex_expect("list element scalar was invalid");
369-
}
370-
builder.finish()
371-
})
372-
}
373-
374-
fn append_list_fill<O: IntegerPType, S: IntegerPType>(
375-
builder: &mut ListViewBuilder<O, S>,
376-
fill_elements: Option<&ArrayRef>,
346+
/// Appends the run of `count` fill values that covers the gap before the next patch.
347+
///
348+
/// The run goes in as a single [`ConstantArray`] rather than one appended value per row. For a
349+
/// list dtype that is the difference between storing the fill value once per gap and once per row:
350+
/// canonicalizing a constant list array points every view at one copy of the value, and the
351+
/// builder keeps that layout.
352+
fn append_fill(
353+
builder: &mut dyn ArrayBuilder,
354+
fill_value: &Scalar,
377355
count: usize,
378356
ctx: &mut ExecutionCtx,
379357
) {
380-
if let Some(fill_elements) = fill_elements {
381-
for _ in 0..count {
382-
builder
383-
.append_array_as_list(fill_elements, ctx)
384-
.vortex_expect("Failed to append sparse fill value");
385-
}
386-
} else {
387-
builder.append_nulls(count);
358+
if count == 0 {
359+
return;
388360
}
389-
}
390361

391-
fn append_fixed_size_list_fill(
392-
builder: &mut FixedSizeListBuilder,
393-
fill_elements: Option<&ArrayRef>,
394-
count: usize,
395-
ctx: &mut ExecutionCtx,
396-
) {
397-
if let Some(fill_elements) = fill_elements {
398-
for _ in 0..count {
399-
builder
400-
.append_array_as_list(fill_elements, ctx)
401-
.vortex_expect("Failed to append sparse fixed-size-list fill value");
402-
}
403-
} else {
362+
if fill_value.is_null() {
363+
// A null fill has no elements to share, and the builder can record the nulls without
364+
// going through an array at all.
404365
builder.append_nulls(count);
366+
return;
405367
}
368+
369+
ConstantArray::new(fill_value.clone(), count)
370+
.into_array()
371+
.append_to_builder(builder, ctx)
372+
.vortex_expect("Failed to append sparse fill value");
406373
}
407374

408375
fn execute_sparse_bools(
@@ -1303,6 +1270,60 @@ mod test {
13031270
Ok(())
13041271
}
13051272

1273+
/// Each gap between patches is appended as a single constant array, so the fill value's
1274+
/// elements are stored once per gap however many rows the gap covers.
1275+
#[test]
1276+
fn test_sparse_list_fill_stores_one_copy_per_gap() -> VortexResult<()> {
1277+
let mut ctx = SESSION.create_execution_ctx();
1278+
1279+
// Two single-element patch lists: [1] and [2].
1280+
let lists = unsafe {
1281+
ListViewArray::new_unchecked(
1282+
buffer![1i32, 2].into_array(),
1283+
buffer![0u32, 1].into_array(),
1284+
buffer![1u32, 1].into_array(),
1285+
Validity::AllValid,
1286+
)
1287+
.with_zero_copy_to_list(true)
1288+
}
1289+
.into_array();
1290+
1291+
// Patches at 10 and 20 of 10,000 rows, so the fill covers three gaps.
1292+
let indices = buffer![10u32, 20].into_array();
1293+
let fill = vec![7i32, 8, 9];
1294+
let sparse =
1295+
Sparse::try_new(indices, lists, 10_000, Scalar::from(Some(fill.clone())))?.into_array();
1296+
1297+
let actual = sparse.execute::<ListViewArray>(&mut ctx)?;
1298+
assert_eq!(actual.len(), 10_000);
1299+
assert_eq!(
1300+
actual.elements().len(),
1301+
2 + 3 * fill.len(),
1302+
"the fill value should be stored once per gap, not once per row",
1303+
);
1304+
1305+
let fill_elements = PrimitiveArray::from_iter(fill);
1306+
for index in [0, 9, 11, 19, 21, 9_999] {
1307+
assert_arrays_eq!(
1308+
actual.list_elements_at(index).vortex_expect("fill list"),
1309+
fill_elements,
1310+
&mut ctx
1311+
);
1312+
}
1313+
assert_arrays_eq!(
1314+
actual.list_elements_at(10).vortex_expect("patch list"),
1315+
PrimitiveArray::from_iter([1i32]),
1316+
&mut ctx
1317+
);
1318+
assert_arrays_eq!(
1319+
actual.list_elements_at(20).vortex_expect("patch list"),
1320+
PrimitiveArray::from_iter([2i32]),
1321+
&mut ctx
1322+
);
1323+
1324+
Ok(())
1325+
}
1326+
13061327
#[test]
13071328
fn test_sparse_binary_varbin_null_fill() {
13081329
let mut ctx = SESSION.create_execution_ctx();

0 commit comments

Comments
 (0)