[Variant] `VariantArray::data_type` returns `StructType`, causing `Array::as_struct` to panic

[scovich]

Describe the bug

arrow-rs normally has a 1:1 relationship between data types and array types, in order to allow downcasting from &dyn Array or ArrayRef. But VariantArray breaks that convention by declaring it is a StructType.

To Reproduce

A match of the form:

match array.data_type() {
      ...
    StructType(_) => array.as_struct().do_something(), // panics!
      ...
}

Expected behavior

Somehow we need a safe way to distinguish a VariantArray from a normal StructArray. Not just to protect hapless uses, but also to make our own code safe (cast_to_variant would both panic if passed a VariantArray today).

Additional context

Discovered during pathfinding work for variant shredding and a potential extension type for variant.

[scovich]

CC @alamb

[alamb]

Added to #6736

Yes, I am not sure how to handle this yet. I think I hit the same issue when trying to write variant to a parquet file:

• [Variant] writing a VariantArray to parquet panics #8296

I will do some research and figure out how to fix this

[alamb]

I did some research, and TLDR I think the solution to this problem is to not have VariantArray implement Array (we'll keep the same methods just not the trait impl)

And then we will add methods and documentation for explicitly converting to/from VariantArray/StructArray

We'll also add a "extension type" annotation on the StructArray's field, when relevant, so it will be propagated through parquet.

This will ensure we can read / write to parquet as parquet itself won't do anything except ensure the field metadata is set appropriately

I am planning to use the model used by @kylebarron in geoarrow. For example https://github.com/geoarrow/geoarrow-rs/blob/b771e889114141a9a3d382e126cccd2fd53f12de/rust/geoarrow-schema/src/datatype.rs#L352-L354

And you access the extension type fields like this:

field.extension_type_name().ok_or(GeoArrowError::InvalidGeoArrow(
                "Expected GeoArrow extension metadata, but found none, and `require_geoarrow_metadata` is `true`.".to_string(),
            ))?;

[kylebarron]

not have VariantArray implement Array

Indeed, for a while I wanted to make my extension arrays implement Array and I concluded it wasn't possible.

I figure there's a canonical extension type for Variant, right?

[scovich]

I figure there's a canonical extension type for Variant, right?

That's the goal (it is part of parquet, after all). But we've not defined the extension type at all yet, let alone made it a canonical extension.

But I don't think canonical extension types solve the problem, because they are encoded at Field level, not DataType. Which is ~useless to the myriad code sites that use Array::data_type to guide array downcasting decisions.

That's why there's no JsonArray: Array or UuidArray: Array type for example. Making the extension type canonical just makes it easily available to users because it's centrally defined.

[scovich]

It seems like no matter what, a variant column is is just a plain old ordinary StructType and StructArray.

We could take several approaches to the variant extension type (not necessarily mutually exclusive):

1. Whatever schema/struct/map/list contains the variant column annotates the column's field as parquet.variant.
2. Inside a shredded variant, the typed_value field of a (partially) shredded variant object field is annotated as parquet.variant.shredded-object-field and the typed_value field of a shredded variant array element is annotated as parquet.variant.shredded-array-element.
3. The metadata and value fields of that struct are themselves extension types (e.g. parquet.variant.metadata and parquet.variant.binary, respectively)

1/ is probably the most intuitive way for users to track variant columns in their schemas and data.

• So we probably want to do that (following the example of other canonical extension types).
• But if we only implement /1, I'm not sure it gives us as much "protection" as we'd like?
• For example, cast_to_variant takes &dyn Array as input, and would be incurably vulnerable to a user who passes a variant column (either accidentally, or intentionally hoping to unshred it).
• Fortunately, variant_get takes a FieldRef and so can conceivably be taught to handle a request to extract variant data (binary or shredded) from the input (including a request to extract a struct with variant fields).

If we do 1/, we probably want to do 2/ as well.

• Shredded object fields and array elements have different structure and null-handling semantics than top-level variant columns do, even tho they are otherwise quite similar.
• On the other hand, we can at least know from context when to expect (or not expect) a shredded object field or array element, as long as we reliably know when we start traversing a (possibly shredded) variant column.

If we do 3/, then we can identify variant data even if the caller forgot to check the field that contained the struct array or struct type they passed us.

• One could imagine that it's enough to only do 3/, tho we might have to do more reverse engineering work when working with variant data.
• However, there's additional complexity because now extension types are buried deep within a nested top-level extension type. I could imagine that causing some issues if we're not careful? A similar risk applies to 2/ as well, now that I think about it.

This probably needs some pathfinding...

[alamb]

I figure there's a canonical extension type for Variant, right?

It is in the final phases of completion, see:

• [Format] Add an Arrow Canonical Extension Type for Parquet Variant arrow#46908

That's the goal (it is part of parquet, after all). But we've not defined the extension type at all yet, let alone made it a canonical extension.

I think @kylebarron is referring to an arrow canonical extension type (e.g. https://arrow.apache.org/docs/format/CanonicalExtensions.html) which is different than the Parquet type

[alamb]

1/ is probably the most intuitive way for users to track variant columns in their schemas and data.

I agree and in my mind this is what @kylebarron has done in geoarrow

Inside a shredded variant, the typed_value field of a (partially) shredded variant object field is annotated as parquet.variant.shredded-object-field and the typed_value field of a shredded variant array element is annotated as parquet.variant.shredded-array-element.

I am not quite sure about this

For example, cast_to_variant takes &dyn Array as input, and would be incurably vulnerable to a user who passes a variant column (either accidentally, or intentionally hoping to unshred it).

Another possible approach here is some new trait or enum, something like

enum VariantOrArray {
  // result is a variant array
  VariantArray(VariantArray),
  // result is some other arrow type
  Array(ArrayRef)
}

I have to think more carefully about what cast_to_variant is doing

This probably needs some pathfinding...

Yes, I will do so.

[alamb]

Update here is that removing the impl Array from VariantArray seems to have worked quite well.

I am now completing some end to end tests showing how to read/write VariantArrays to/from parquet files with the correct LogicalType annotations, and then I'll get some PRs ready for review