We might be able to move this into variant_to_arrow, and variant_get could get the support out of the box. Given the size of this PR, I would do that as a followup.

This is great!

Double checking -- if I try to shred as List<i32> and I encounter a variant array [..., "hi", ...], the bad entry will either become NULL or cause an error, depending on cast options?

Seems the hi will be located in typed_value.value

Seems the hi will be located in typed_value.value

that's right. For example, an variant array [1, 2, 3, 1, "two", Variant::Null] would become

typed_value: [1, 2, 3, 1, null, null]
value: [null, null, null, null, "two", Variant::Null]

Can refer to test_array_shredding_as_list as an example

Can we introduce a ListLikeArrayBuilder trait (**) that encapsulates the (minimal) differences between these four types, so that ArrayVariantToArrowRowBuilder becomes a generic struct instead of an enum?

(**) c.f. StringLikeArrayBuilder that serves the same purpose for strings

A quick analysis suggests the trait needs:

• An associated type: type Offset: OffsetSizeTrait
• A constructor: fn try_new(...) -> Result<Self>
• Helper functions to support append_null and append_value (nulls, offsets, etc)
• A finisher: fn finish(self) -> Result<ArrayRef>

Two trait implementations (one for lists and one for list views), both generic over Offset

And from there, the outer builder should be able to implement its own logic just once instead of four times.

Double check tho -- the above is a very rough sketch. The goal is to minimize boilerplate and duplication, using a careful selection of trait methods that capture the essential differences between lists and list views.

I tried to refactor the code using ListLikeArrayBuilder, but it doesn’t help much (draft PR). Plus, it would also force the builder into a boxed dyn trait, adding overhead versus the enum approach.

I think the current impl actaully follow what we did in varaint_to_arrow, the code structure looks like below

Variant (VariantToShreddedVariantRowBuilder)
├─ Primitive (VariantToShreddedPrimitiveVariantRowBuilder)
│  ├─ Int/Float group (PrimitiveVariantToArrowRowBuilder::VariantToPrimitiveArrowRowBuilder):
│  │    Null, Boolean, Int8/16/32/64, UInt8/16/32/64,
│  │    Float16/32/64, Decimal32/64/128/256
│  ├─ Time-like (PrimitiveVariantToArrowRowBuilder::VariantToTimestampArrowRowBuilder):
│  │    Date32, Time64(us), Timestamp(us/ns) [tz or ntz]
│  └─ String/Binary (PrimitiveVariantToArrowRowBuilder::VariantToStringArrowRowBuilder/VariantToBinaryArrowRowBuilder):
│       Utf8, Utf8View, Binary, BinaryView
├─ Array (VariantToShreddedArrayVariantRowBuilder)
│  ├─ List          (ArrayVariantToArrowRowBuilder::VariantToListArrowRowBuilder)
│  ├─ LargeList     (ArrayVariantToArrowRowBuilder::VariantToListArrowRowBuilder)
│  ├─ ListView      (ArrayVariantToArrowRowBuilder::VariantToListViewArrowRowBuilder)
│  └─ LargeListView (ArrayVariantToArrowRowBuilder::VariantToListViewArrowRowBuilder)
│      └─ each element uses make_variant_to_shredded_variant_arrow_row_builder recursively
└─ Object (VariantToShreddedObjectVariantRowBuilder)

StringLikeArrayBuilder and BinaryLikeArrayBuilder mainly simplify VariantToStringArrowRowBuilder and VariantToBinaryArrowRowBuilder but still need dispatch from PrimitiveVariantToArrowRowBuilder. In theory, ListLikeArrayBuilder could simplify nulls, offsets, and sizes via GenericList(View)Builder, but we’d need a lot of adapters, because element_builder is Box<VariantToShreddedVariantRowBuilder<'a>> and doesn’t implement ArrayBuilder directly to make the trait work in our case.

Can we unify the VariantToListViewArrowRowBuilder and VariantToListArrowRowBuilder here if introducing ListLikeArrayBuilder, like PrimitiveVariantToArrowRowBuilder(VariantToPrimitiveArrowRowBuilder<'a, T: PrimitiveFromVariant>) ?

StringLikeArrayBuilder and BinaryLikeArrayBuilder mainly simplify VariantToStringArrowRowBuilder and VariantToBinaryArrowRowBuilder but still need dispatch from PrimitiveVariantToArrowRowBuilder. In theory, ListLikeArrayBuilder could simplify nulls, offsets, and sizes via GenericList(View)Builder, but we’d need a lot of adapters, because element_builder is Box<VariantToShreddedVariantRowBuilder<'a>> and doesn’t implement ArrayBuilder directly to make the trait work in our case.

I think the problem is the draft PR still left the hierarchical builder thing in place? Double enum dispatch? I thought the other -like traits eliminated the double dispatch. The top-level enum has an enum variant for each concrete type, but each one is just instantiating a different version of the same generic builder?

(I wouldn't have expected the inner builder to be related to this refactor)

Emmm, I don't think there is a double enum dispatch in this PR, this is the only place where we match the DataType to specific builders. ArrayVariantToArrowRowBuilder is on the same level as PrimitiveVariantToArrowRowBuilder and ObjectVariantToArrowRowBuilder

In PrimitiveVariantToArrowRowBuilder (top-level enum):

And this PR did the same thing here in ArrayVariantToArrowRowBuilder, where this top-level enum dispatches 4 different list builders for each concrete list types.

BTW, I updated the draft PR to make ArrayVariantToArrowRowBuilder fairly slim, but it basically moves the match data_type from this builder to the caller side and does not reduce duplicate much.

Can we unify the VariantToListViewArrowRowBuilder and VariantToListArrowRowBuilder here if introducing ListLikeArrayBuilder, like PrimitiveVariantToArrowRowBuilder(VariantToPrimitiveArrowRowBuilder<'a, T: PrimitiveFromVariant>) ?

Indeed we can if we make it the same way like StringLikeArrayBuilder/BinaryLikeArrayBuilder, then we don't need to maintain nulls, offsets, and sizes here. But it needs some adapters because element_builder is Box<VariantToShreddedVariantRowBuilder<'a>> and doesn’t implement ArrayBuilder directly to make the trait work in our case. I may use another PR to see if we can easily simplify the code here

Seems the hi will be located in typed_value.value

Can we unify the VariantToListViewArrowRowBuilder and VariantToListArrowRowBuilder here if introducing ListLikeArrayBuilder, like PrimitiveVariantToArrowRowBuilder(VariantToPrimitiveArrowRowBuilder<'a, T: PrimitiveFromVariant>) ?

This is super similar to the variant list builder. AFAICT, the only meaningful difference is how offsets and sizes are handled? And for this specific use case, we could compute both from the normal offsets array, e.g.:

offsets: [0, a, b, ..., y, z]

translates in view form to

offsets: [0, a, b, ..., y]
sizes: [a-0, b-a, ..., z-y]

Thus, a single implementation (generic over const IS_VIEW: bool) could have a uniform implementation for everything except the final array build?

if IS_VIEW {
    let n = self.offsets.len();
    let sizes = Vec::with_capacity(n - 1)
    for i in 1..n {
        sizes.push(self.offsets[i] - self.offsets[i-1]);
    }
    self.offsets.pop(); // drop the now-unneeded extra entry
    // create a ListViewArray from offsets and sizes
} else {
    // create a ListArray from offsets
}

The only downside is allocating one extra element of capacity in case of a list view... but that seems highly unlikely to matter in practice?

Makes sense, we can merge these 2 builders with the new const