[Variant] Remove potential panics when probing `VariantArray`

[friendlymatthew]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Consider the following:

 fn foo() {
    let mut b = VariantArrayBuilder::new(1);
    b.append_null();
    let v = b.build();

    v.value(0); // will panic
}

Probing a VariantArray when that element is null will result in a panic:

thread 'cast_to_variant::tests::foo' panicked at /Users/matthewkim/.cargo/git/checkouts/arrow-rs-583cca34693b79b8/760b7b6/parquet-variant/src/variant.rs:318:14:
Invalid variant metadata: InvalidArgumentError("Received empty bytes")

This seems like an odd pattern, as it's not clear exactly what a user should do when iterating or probing through a VariantArray

Describe the solution you'd like
We could have VariantArray::value(i: usize) return an Option<Variant<'a>>. This way, it's clear when it's None, that element is considered null. Note, this is different from Some(Variant::Null))

Additional context

This feature is motivated by datafusion-variant, a higher-level library that integrates with Datafusion through a series of udfs

[friendlymatthew]

This would be solved via http://github.com/apache/arrow-rs/issues/8612

I noticed the solution I described isn't necessarily idiomatic to the array convention. Option<T> is usually delegated to iter() calls on the array itself

[scovich]

I think you've identified two separate issues:

Probing a VariantArray when that element is null will result in a panic

This is arguably a bug, and the immediate fix is likely a one-liner:

    pub fn value(&self, index: usize) -> Variant<'_, '_> {
        match (self.typed_value_field(), self.value_field()) {
            // Always prefer typed_value, if available                                                                                                                                                                                                                         
            (Some(typed_value), value) if typed_value.is_valid(index) => {
                typed_value_to_variant(typed_value, value, index)
            }
            // Otherwise fall back to value, if available                                                                                                                                                                                                                      
-           (_, Some(value)) if value.is_valid(index) => {
+           (_, Some(value)) if self.metadata.is_valid(index) && value.is_valid(index) => {
                Variant::new(self.metadata.value(index), value.value(index))
            }
            // It is technically invalid for neither value nor typed_value fields to be available,                                                                                                                                                                             
            // but the spec specifically requires readers to return Variant::Null in this case.                                                                                                                                                                                
            _ => Variant::Null,
        }

... but only because VariantArray::typed_value_to_variant doesn't support partially shredded objects yet.

Once we support those, that helper will need to take the metadata column as an additional arg, and the lifetime annotations will likely need to change as well.

Actually... I think the method cannot support partially shredded objects because Variant is fundamentally a reference type, and returning a single Variant for a partially shredded object would require reassembling it.

Really, we'd be forced to add the new Variant::ShreddedObject enum variant that @alamb has mentioned a couple of times as a way to capture all the raw bytes as-is, without trying to reassemble them.

If we always unshred all primitive types, then it might be as simple as adding new enum variants ShreddedObject(ShreddedVariantObject) and ShreddedList(Vec<Variant<'m, 'v>) where:

struct ShreddedVariantObject<'m, 'v> {
    unshredded_fields: Option<VariantObject<'m, 'v>>,
    shredded_fields: Vec<(&'m str, Variant<'m, 'v>)>,
}

The hard part is populating it, not defining it...

Meanwhile, there are some other panics lurking in that method, which we should probably track fixes for as well?

This seems like an odd pattern, as it's not clear exactly what a user should do when iterating or probing through a VariantArray

For any arrow array, a caller who cares about nulls needs to check Array::is_null or Array::is_valid before invoking the value method. A blind v.value(0) is never recommended (but it still shouldn't panic).