Skip to content

Commit ce2eb1d

Browse files
clauderobert3005
authored andcommitted
refactor(array): canonicalize chunked structs and FSLs through the builder
`pack_struct_chunks` and `swizzle_fixed_size_list_chunks` existed because `append_to_builder` used to decode a builder's children: without them, canonicalizing a `ChunkedArray` would concatenate every chunk's children instead of reusing them. Their doc comments describe what the builders now do on their own, so both are dead weight. Route both dtypes through the generic builder path. `ListArray` keeps its swizzle for now — that one also allocates its offsets and sizes through the session allocator, which the builders cannot yet do. 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 7b61176 commit ce2eb1d

1 file changed

Lines changed: 54 additions & 60 deletions

File tree

‎vortex-array/src/arrays/chunked/vtable/canonical.rs‎

Lines changed: 54 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,10 @@ use crate::IntoArray;
1515
use crate::array::ArrayView;
1616
use crate::arrays::Chunked;
1717
use crate::arrays::ChunkedArray;
18-
use crate::arrays::FixedSizeListArray;
1918
use crate::arrays::ListViewArray;
2019
use crate::arrays::PrimitiveArray;
21-
use crate::arrays::StructArray;
2220
use crate::arrays::VariantArray;
2321
use crate::arrays::chunked::ChunkedArrayExt;
24-
use crate::arrays::fixed_size_list::FixedSizeListArraySlotsExt;
2522
use crate::arrays::listview::ListViewArraySlotsExt;
2623
use crate::arrays::listview::ListViewRebuildMode;
2724
use crate::arrays::variant::VariantArraySlotsExt;
@@ -48,28 +45,20 @@ pub(super) fn _canonicalize(
4845
return array.chunk(0).clone().execute::<Canonical>(ctx);
4946
}
5047

51-
let owned_chunks: Vec<ArrayRef> = array.iter_chunks().cloned().collect();
5248
Ok(match array.dtype() {
53-
DType::Struct(..) => {
54-
let struct_array = pack_struct_chunks(owned_chunks, ctx)?;
55-
Canonical::Struct(struct_array)
56-
}
57-
DType::List(elem_dtype, _) => Canonical::List(swizzle_list_chunks(
58-
&owned_chunks,
59-
array.array().validity()?,
60-
elem_dtype,
61-
ctx,
62-
)?),
63-
DType::FixedSizeList(elem_dtype, list_size, _) => {
64-
Canonical::FixedSizeList(swizzle_fixed_size_list_chunks(
49+
DType::List(elem_dtype, _) => {
50+
let owned_chunks: Vec<ArrayRef> = array.iter_chunks().cloned().collect();
51+
Canonical::List(swizzle_list_chunks(
6552
&owned_chunks,
6653
array.array().validity()?,
6754
elem_dtype,
68-
*list_size,
6955
ctx,
7056
)?)
7157
}
72-
DType::Variant(_) => Canonical::Variant(pack_variant_chunks(owned_chunks, ctx)?),
58+
DType::Variant(_) => {
59+
let owned_chunks: Vec<ArrayRef> = array.iter_chunks().cloned().collect();
60+
Canonical::Variant(pack_variant_chunks(owned_chunks, ctx)?)
61+
}
7362
_ => {
7463
let mut builder = builder_with_capacity_in(ctx.allocator(), array.dtype(), array.len());
7564
array.array().append_to_builder(builder.as_mut(), ctx)?;
@@ -78,17 +67,6 @@ pub(super) fn _canonicalize(
7867
})
7968
}
8069

81-
/// Packs many [`StructArray`]s to instead be a single [`StructArray`], where the [`DynArrayData`](crate::array::DynArrayData) for each
82-
/// field is a [`ChunkedArray`].
83-
///
84-
/// The caller guarantees there are at least 2 chunks.
85-
fn pack_struct_chunks(chunks: Vec<ArrayRef>, ctx: &mut ExecutionCtx) -> VortexResult<StructArray> {
86-
chunks
87-
.into_iter()
88-
.map(|c| c.execute::<StructArray>(ctx))
89-
.process_results(|iter| StructArray::try_concat(iter))?
90-
}
91-
9270
/// Packs many [`VariantArray`]s into one [`VariantArray`] with chunked children.
9371
///
9472
/// The caller guarantees there are at least 2 chunks.
@@ -253,45 +231,16 @@ fn swizzle_list_chunks(
253231
})
254232
}
255233

256-
/// Packs [`FixedSizeListArray`]s together into a single [`FixedSizeListArray`] whose `elements`
257-
/// child is a [`ChunkedArray`].
258-
///
259-
/// Every chunk shares the same `list_size`, and each chunk's `elements` child is exactly
260-
/// `list_size * chunk.len()` long and starts at the first list, so we can reuse the chunks'
261-
/// `elements` children directly as the chunks of a combined `elements` array without copying.
262-
///
263-
/// The caller guarantees there are at least 2 chunks.
264-
fn swizzle_fixed_size_list_chunks(
265-
chunks: &[ArrayRef],
266-
validity: Validity,
267-
elem_dtype: &DType,
268-
list_size: u32,
269-
ctx: &mut ExecutionCtx,
270-
) -> VortexResult<FixedSizeListArray> {
271-
let len: usize = chunks.iter().map(|c| c.len()).sum();
272-
273-
let mut element_chunks = Vec::with_capacity(chunks.len());
274-
for chunk in chunks {
275-
let chunk_array = chunk.clone().execute::<FixedSizeListArray>(ctx)?;
276-
// A canonical `FixedSizeListArray` keeps its `elements` child trimmed to exactly
277-
// `list_size * chunk.len()` starting at the first list, so the children concatenate
278-
// cleanly into the combined `elements` array.
279-
element_chunks.push(chunk_array.elements().clone());
280-
}
281-
282-
let chunked_elements = ChunkedArray::try_new(element_chunks, elem_dtype.clone())?.into_array();
283-
284-
FixedSizeListArray::try_new(chunked_elements, list_size, validity, len)
285-
}
286-
287234
#[cfg(test)]
288235
mod tests {
289236
use std::sync::Arc;
290237
use std::sync::LazyLock;
291238
use std::sync::atomic::AtomicUsize;
292239
use std::sync::atomic::Ordering;
293240

241+
use rstest::rstest;
294242
use vortex_buffer::buffer;
243+
use vortex_error::VortexExpect;
295244
use vortex_error::VortexResult;
296245
use vortex_error::vortex_bail;
297246
use vortex_error::vortex_err;
@@ -301,15 +250,20 @@ mod tests {
301250
use crate::Canonical;
302251
use crate::IntoArray;
303252
use crate::VortexSessionExecute;
253+
use crate::arrays::Chunked;
304254
use crate::arrays::ChunkedArray;
305255
use crate::arrays::ConstantArray;
256+
use crate::arrays::FixedSizeList;
306257
use crate::arrays::FixedSizeListArray;
307258
use crate::arrays::ListArray;
308259
use crate::arrays::ListViewArray;
309260
use crate::arrays::PrimitiveArray;
261+
use crate::arrays::Struct;
310262
use crate::arrays::StructArray;
311263
use crate::arrays::VarBinViewArray;
312264
use crate::arrays::VariantArray;
265+
use crate::arrays::chunked::ChunkedArrayExt;
266+
use crate::arrays::fixed_size_list::FixedSizeListArraySlotsExt;
313267
use crate::arrays::struct_::StructArrayExt;
314268
use crate::arrays::variant::VariantArraySlotsExt;
315269
use crate::assert_arrays_eq;
@@ -660,6 +614,46 @@ mod tests {
660614
Ok(())
661615
}
662616

617+
/// Canonicalizing a `ChunkedArray` reuses each chunk's children instead of concatenating
618+
/// them, which is why the builder is told to keep every chunk boundary. The chunks here are
619+
/// far shorter than the threshold a nested builder would apply on its own.
620+
#[rstest]
621+
#[case::struct_(
622+
StructArray::try_from_iter([("a", buffer![1i32, 2])])
623+
.vortex_expect("struct array")
624+
.into_array(),
625+
|array: &ArrayRef| array.as_::<Struct>().unmasked_field(0).clone()
626+
)]
627+
#[case::fixed_size_list(
628+
FixedSizeListArray::new(buffer![1i32, 2].into_array(), 2, Validity::NonNullable, 1)
629+
.into_array(),
630+
|array: &ArrayRef| array.as_::<FixedSizeList>().elements().clone()
631+
)]
632+
fn canonicalize_reuses_chunk_children(
633+
#[case] chunk: ArrayRef,
634+
#[case] child_of: fn(&ArrayRef) -> ArrayRef,
635+
) -> VortexResult<()> {
636+
let mut ctx = SESSION.create_execution_ctx();
637+
638+
let chunked =
639+
ChunkedArray::try_new(vec![chunk.clone(), chunk.clone()], chunk.dtype().clone())?
640+
.into_array();
641+
let canonical = chunked.execute::<Canonical>(&mut ctx)?.into_array();
642+
643+
let child = child_of(&canonical);
644+
assert_eq!(
645+
child.as_::<Chunked>().nchunks(),
646+
2,
647+
"each chunk's child should have become a chunk of the combined child",
648+
);
649+
650+
let expected =
651+
ChunkedArray::try_new(vec![chunk.clone(), chunk], canonical.dtype().clone())?;
652+
assert_arrays_eq!(&canonical, &expected, &mut ctx);
653+
654+
Ok(())
655+
}
656+
663657
#[test]
664658
fn list_canonicalize_uses_memory_session_allocator() {
665659
let allocations = Arc::new(AtomicUsize::new(0));

0 commit comments

Comments
 (0)