Skip to content

Commit 947a966

Browse files
committed
Handle any list builder kind in append_to_builder
`builder_with_capacity` produces a `ListViewBuilder` for `DType::List`, but `List::append_to_builder` downcast the builder to `ListBuilder<u64>` and bailed otherwise — so executing a `List`-encoded array into the canonical builder (the one `builder_with_capacity` returns) failed. `ListView::append_to_builder` had the mirror-image assumption, only accepting a `ListViewBuilder<u64, u64>`. Add two macros that enumerate the possible list builder offset/size integer types and try downcasting to each, rather than assuming the types produced by `builder_with_capacity`: - `match_each_list_builder!` enumerates `ListBuilder<O>`. - `match_each_listview_builder!` enumerates `ListViewBuilder<O, S>`. `List::append_to_builder` tries `ListBuilder` (its non-canonical, directly-supplied builder) and otherwise canonicalizes and dispatches, routing the canonical `ListViewBuilder` case through `ListView::append_to_builder`, which tries `ListViewBuilder`. Either encoding works with any unsigned offset/size types. Other encodings that yield list dtypes (constant, dict, chunked, ...) already route here through the canonical-dispatch fallback. Signed-off-by: Robert <robert@spiraldb.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mkrgj5SuJpaKBXK9jydtSc
1 parent d7c69bd commit 947a966

4 files changed

Lines changed: 125 additions & 16 deletions

File tree

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use vortex_session::registry::CachedId;
1616
use crate::ArrayEq;
1717
use crate::ArrayHash;
1818
use crate::ArrayRef;
19+
use crate::Canonical;
1920
use crate::EqMode;
2021
use crate::ExecutionCtx;
2122
use crate::ExecutionResult;
@@ -35,7 +36,7 @@ use crate::arrays::list::compute::rules::PARENT_RULES;
3536
use crate::arrays::listview::list_view_from_list;
3637
use crate::buffer::BufferHandle;
3738
use crate::builders::ArrayBuilder;
38-
use crate::builders::ListBuilder;
39+
use crate::builders::match_each_list_builder;
3940
use crate::dtype::DType;
4041
use crate::dtype::Nullability;
4142
use crate::dtype::PType;
@@ -198,10 +199,14 @@ impl VTable for List {
198199
builder: &mut dyn ArrayBuilder,
199200
ctx: &mut ExecutionCtx,
200201
) -> VortexResult<()> {
201-
let Some(builder) = builder.as_any_mut().downcast_mut::<ListBuilder<u64>>() else {
202-
vortex_bail!("append_to_builder for List requires a ListBuilder<u64, u64>");
203-
};
204-
builder.append_list_array(&array.into_owned().into_array(), ctx)
202+
let array = array.into_owned().into_array();
203+
// A directly-supplied `ListBuilder` (not the canonical builder for `DType::List`).
204+
match_each_list_builder!(&array, builder, ctx);
205+
// The canonical builder is `ListViewBuilder`; canonicalize and dispatch to `ListView`.
206+
array
207+
.execute::<Canonical>(ctx)?
208+
.into_array()
209+
.append_to_builder(builder, ctx)
205210
}
206211
}
207212

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

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use crate::ArrayRef;
2020
use crate::EqMode;
2121
use crate::ExecutionCtx;
2222
use crate::ExecutionResult;
23+
use crate::IntoArray;
2324
use crate::array::Array;
2425
use crate::array::ArrayId;
2526
use crate::array::ArrayView;
@@ -34,7 +35,7 @@ use crate::arrays::listview::array::SLOT_NAMES;
3435
use crate::arrays::listview::compute::rules::PARENT_RULES;
3536
use crate::buffer::BufferHandle;
3637
use crate::builders::ArrayBuilder;
37-
use crate::builders::ListViewBuilder;
38+
use crate::builders::match_each_listview_builder;
3839
use crate::dtype::DType;
3940
use crate::dtype::Nullability;
4041
use crate::dtype::PType;
@@ -222,14 +223,12 @@ impl VTable for ListView {
222223
builder: &mut dyn ArrayBuilder,
223224
ctx: &mut ExecutionCtx,
224225
) -> VortexResult<()> {
225-
// `builder_with_capacity` always produces a `ListViewBuilder<u64, u64>` for `DType::List`.
226-
let Some(builder) = builder
227-
.as_any_mut()
228-
.downcast_mut::<ListViewBuilder<u64, u64>>()
229-
else {
230-
vortex_bail!("append_to_builder for ListView requires a ListViewBuilder<u64, u64>");
231-
};
232-
builder.append_listview_array(&array.into_owned(), ctx)
226+
let array = array.into_owned().into_array();
227+
match_each_listview_builder!(&array, builder, ctx);
228+
vortex_bail!(
229+
"append_to_builder for ListView requires a ListViewBuilder, got a builder for {}",
230+
builder.dtype()
231+
)
233232
}
234233

235234
fn reduce_parent(

vortex-array/src/builders/list.rs

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,9 @@ impl<O: IntegerPType> ListBuilder<O> {
173173
/// [`ListViewArray`] and converting into the `ListArray` (`n + 1` offsets) layout.
174174
///
175175
/// [`ListBuilder`] is not the canonical builder for [`DType::List`] (that is
176-
/// [`ListViewBuilder`](crate::builders::ListViewBuilder)), so no encoding dispatches into it via
177-
/// `append_to_builder`; this helper exists for direct callers such as tests.
176+
/// [`ListViewBuilder`](crate::builders::ListViewBuilder)), so it is never produced by
177+
/// [`builder_with_capacity`]. List encodings still dispatch into it via `match_each_list_builder`
178+
/// when a caller supplies one directly.
178179
pub(crate) fn append_list_array(
179180
&mut self,
180181
array: &ArrayRef,
@@ -334,6 +335,7 @@ mod tests {
334335
use Nullability::Nullable;
335336
use vortex_buffer::buffer;
336337
use vortex_error::VortexExpect;
338+
use vortex_error::VortexResult;
337339

338340
use crate::IntoArray;
339341
use crate::array_session;
@@ -344,6 +346,8 @@ mod tests {
344346
use crate::arrays::listview::ListViewArrayExt;
345347
use crate::assert_arrays_eq;
346348
use crate::builders::ArrayBuilder;
349+
use crate::builders::ListViewBuilder;
350+
use crate::builders::builder_with_capacity;
347351
use crate::builders::list::ListArray;
348352
use crate::builders::list::ListBuilder;
349353
use crate::dtype::DType;
@@ -514,6 +518,50 @@ mod tests {
514518
);
515519
}
516520

521+
/// `append_to_builder` must handle any list builder kind without assuming the offset/size
522+
/// integer types produced by `builder_with_capacity`. It appends a `List`-encoded array and a
523+
/// `ListView`-encoded array into `ListViewBuilder`s and `ListBuilder`s with assorted (and
524+
/// non-`u64`) offset/size types.
525+
#[test]
526+
fn test_append_to_builder_any_list_builder() -> VortexResult<()> {
527+
let mut ctx = array_session().create_execution_ctx();
528+
529+
let list = ListArray::from_iter_opt_slow::<u64, _, _>(
530+
[Some(vec![0, 1, 2]), None, Some(vec![4, 5])],
531+
Arc::new(I32.into()),
532+
)?
533+
.into_array();
534+
let listview = list
535+
.clone()
536+
.execute::<ListViewArray>(&mut ctx)?
537+
.into_array();
538+
let elem_dtype = || Arc::new(I32.into());
539+
540+
// `builder_with_capacity` produces a `ListViewBuilder` for `DType::List`; appending the
541+
// `List`-encoded array must dispatch into it instead of bailing.
542+
let mut listview_builder = builder_with_capacity(list.dtype(), list.len());
543+
list.append_to_builder(listview_builder.as_mut(), &mut ctx)?;
544+
assert_arrays_eq!(listview_builder.finish(), list, &mut ctx);
545+
546+
// Offset/size types are enumerated, not assumed: a `ListViewBuilder` with non-`u64`
547+
// offset and size types must work for both source encodings.
548+
let mut lv_u32_u8 = ListViewBuilder::<u32, u8>::with_capacity(elem_dtype(), Nullable, 8, 4);
549+
list.append_to_builder(&mut lv_u32_u8, &mut ctx)?;
550+
assert_arrays_eq!(lv_u32_u8.finish(), list, &mut ctx);
551+
552+
let mut lv_u16_u16 =
553+
ListViewBuilder::<u16, u16>::with_capacity(elem_dtype(), Nullable, 8, 4);
554+
listview.append_to_builder(&mut lv_u16_u16, &mut ctx)?;
555+
assert_arrays_eq!(lv_u16_u16.finish(), list, &mut ctx);
556+
557+
// A `List`-encoded array appended into a `ListBuilder` with a non-`u64` offset type.
558+
let mut list_builder = ListBuilder::<u32>::with_capacity(elem_dtype(), Nullable, 8, 4);
559+
list.append_to_builder(&mut list_builder, &mut ctx)?;
560+
assert_arrays_eq!(list_builder.finish(), list, &mut ctx);
561+
562+
Ok(())
563+
}
564+
517565
#[test]
518566
fn test_extend_builder() {
519567
test_extend_builder_gen::<i8>();

vortex-array/src/builders/mod.rs

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,3 +284,60 @@ pub fn builder_with_capacity_in(
284284
let _allocator = allocator;
285285
builder_with_capacity(dtype, capacity)
286286
}
287+
288+
/// Append a list-typed `$array` into `$builder` if it is a [`ListBuilder`], enumerating the possible
289+
/// offset integer types and trying each downcast. On a match it `return`s the append result from the
290+
/// enclosing function; otherwise it falls through.
291+
///
292+
/// [`ListBuilder`] is never produced by [`builder_with_capacity`] (the canonical list builder is
293+
/// [`ListViewBuilder`]), so its offset type cannot be assumed — hence the enumeration. List offsets
294+
/// are always unsigned integers.
295+
macro_rules! match_each_list_builder {
296+
($array:expr, $builder:expr, $ctx:expr) => {{
297+
$crate::builders::match_each_list_builder!(@types $array, $builder, $ctx, [u8, u16, u32, u64]);
298+
}};
299+
(@types $array:expr, $builder:expr, $ctx:expr, [$($O:ty),*]) => {{
300+
$(
301+
if let Some(builder) = $builder
302+
.as_any_mut()
303+
.downcast_mut::<$crate::builders::ListBuilder<$O>>()
304+
{
305+
return builder.append_list_array($array, $ctx);
306+
}
307+
)*
308+
}};
309+
}
310+
pub(crate) use match_each_list_builder;
311+
312+
/// Append a list-typed `$array` into `$builder` if it is a [`ListViewBuilder`], enumerating the
313+
/// possible offset and size integer types and trying each downcast. On a match it `return`s the
314+
/// append result from the enclosing function; otherwise it falls through.
315+
///
316+
/// [`builder_with_capacity`] produces a `ListViewBuilder<u64, u64>` for [`DType::List`], but a
317+
/// caller may supply different offset/size types, so the concrete type cannot be assumed — hence the
318+
/// enumeration. List offsets and sizes are always unsigned integers.
319+
macro_rules! match_each_listview_builder {
320+
($array:expr, $builder:expr, $ctx:expr) => {{
321+
$crate::builders::match_each_listview_builder!(
322+
@offsets $array, $builder, $ctx, [u8, u16, u32, u64], [u8, u16, u32, u64]
323+
);
324+
}};
325+
// For each offset type `O`, expand over every size type `S` (cartesian product).
326+
(@offsets $array:expr, $builder:expr, $ctx:expr, [$($O:ty),*], $sizes:tt) => {{
327+
$(
328+
$crate::builders::match_each_listview_builder!(@sizes $array, $builder, $ctx, $O, $sizes);
329+
)*
330+
}};
331+
(@sizes $array:expr, $builder:expr, $ctx:expr, $O:ty, [$($S:ty),*]) => {{
332+
$(
333+
if let Some(builder) = $builder
334+
.as_any_mut()
335+
.downcast_mut::<$crate::builders::ListViewBuilder<$O, $S>>()
336+
{
337+
let listview = $array.clone().execute::<$crate::arrays::ListViewArray>($ctx)?;
338+
return builder.append_listview_array(&listview, $ctx);
339+
}
340+
)*
341+
}};
342+
}
343+
pub(crate) use match_each_listview_builder;

0 commit comments

Comments
 (0)