Moved to shred_variant

Here, primitive refers to Variant primitive, not Arrow primitive. So we shouldn’t throw an error about “not an Arrow primitive.” Ideally this function should accept any Arrow type, therefore changed this to NotYetImplemented instead.

We can't make do the type cast after this? not sure if this is ok.

Currently, we will do the type check in make_primitive_variant_to_arrow_row_builder with the match arms, so maybe we don't need to add it here, and add the type check in two places seems will add maintaince.

The issue is that make_primitive_variant_to_arrow_row_builder doesn’t enforce the Parquet-primitive constraint. As a result, types like UInt, which aren’t Parquet primitives, are accepted for shredding because make_primitive_variant_to_arrow_row_builder allows.

Do we need to distinguish this with the _ match arm?

I think this provides a user friendly message and it's actually moved from variant_to_arrow. Don't mind removing it tho

Maybe we can't remove the _ if data_type.is_primitive() for now, seems this arm is for the type we may but have not implement, and _ match arm is for the types invalid.

After #8768 merged, we complete the 1-1 mapping(and some transforms for some types) for all Variant primitive types, but we may support some DataTypes here which is not a valid variant primitive(e.g. Timestamp with different unit), and keep the _ if data_type.is_primitive() so that we know we may support the required, and remove it after we have a conclusion.

I am sorting out possible conversions and will create an issue to discuss them after the work done.

Actually data_type.is_primitive() is not checking Parquet primitive, that's why I wanted to remove it to avoid confusion. It checks arrow primitive which only includes numeric and temporal. If we were to keep it, we shouldn't add Binary or String into this builder as they are not arrow primitive.

My understanding is that

• All Parquet Variant primitive types will map to Arrow DataTypes
• Some Parquet Variant primitive types can be converted to other Arrow DataTypes(with cast)
• Here we want to support all parquet variant primitive types, and the target type here is Arrow DataType, we check the Arrow DataTypes here
• The dataype.is_primitive() check(my guess) is that we can support all Arrow DataTypes, but has not done(when this code written), so add a NotYetImplement here to mark this.

However, it is true that we might need an expert to confirm our understanding here.

• All Parquet Variant primitive types will map to Arrow DataTypes
• Some Parquet Variant primitive types can be converted to other Arrow DataTypes(with cast)
• Here we want to support all parquet variant primitive types, and the target type here is Arrow DataType, we check the Arrow DataTypes here

That's my understanding as well.

• The dataype.is_primitive() check(my guess) is that we can support all Arrow DataTypes, but has not done(when this code written), so add a NotYetImplement here to mark this.

This is why I’m proposing the change. I’m not removing NotYetImplemented. Instead, I’m removing the datatype.is_primitive() check and the InvalidArgumentError: Not a primitive type, because, as you noted, we can support all Arrow DataTypes.

Example: variant_get() with Binary

• Before this change: fails with InvalidArgumentError: Not a primitive type because Binary is not an Arrow primitive. This incorrectly suggests Binary is an invalid target type and shouldn’t be supported.
• After this change: fails with NotYetImplemented, which is the expected behavior until Binary is supported.

Hope this example makes sense

Big question:

I'm pretty sure that Variant::Null (JSON null) is an actual value... but I'm NOT sure how useful it would be in practice. Strict casting would produce an error if even one row had a normal value, and non-strict casting would produce an all-null output no matter what the input was.

So maybe we intentionally forbid DataType::Null, with an explanatory comment?

Update:

I confused myself; the variant shredding spec does not allow shredding as null, so this code is correct as-is.

HOWEVER, while looking at this I realized that we do have a row builder for DataType::Null, and it does not currently enforce strict casting (all values are blindly treated as null, no matter the casting mode).

The simplest fix would be to adjust the null builder's definition:

define_variant_to_primitive_builder!(
     struct VariantToNullArrowRowBuilder<'a>
     |capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },
-    |_value|  Some(Variant::Null),
+    |value|  value.as_null(),
     type_name: "Null"
 );

and change the fake row builder's append_value method to suit:

 impl FakeNullBuilder {
     fn new(capacity: usize) -> Self {
         Self(NullArray::new(capacity))
     }
-    fn append_value<T>(&mut self, _: T) {}
+    fn append_value(&mut self, _: ()) {}
     fn append_null(&mut self) {}

... but that might produce clippy warnings about passing unit type as a function argument. If so, we'd need to adjust the value conversion to produce Some dummy value instead, e.g. value.as_null().map(|_| 0) or matches!(value, Variant::Null).then_some(0)

Also, the fake null builder should probably track how many "values" were "appended" and either produce a NullArray of that length or blow up if the call count disagrees with the array's length. The former is probably more correct than the latter, since it matches all the other builders for whom "capacity" is only a hint.

The latter is unrelated to this PR, tracking it as #8810

Not sure whether this distinction is important enough to merit its own error message?

Also, we eventually need to check the field for UUID extension type and not just rely on the data type. If #8673 merges first, we should fix it here; if this PR merges first the other PR needs to incorporate the change.

CC @friendlymatthew

Hi, so sorry. I swear I never caught this notification. In terms of order, I'm fine with this merging first. I'll read through and update accordingly

lol -- I certainly will never give someone a hard time for missing a notification!

I took a closer look and I think in order to check for an extension type here, we'd need to plumb FieldRef all the way from shred_variant

Meaning shred_variant's header go from:

 shred_variant(array: &VariantArray, as_type: &DataType) -> Result<VariantArray> {}

to

 shred_variant(array: &VariantArray, field_ref: &FieldRef) -> Result<VariantArray> {}

Instead of a catch-all, should we enumerate the remaining data types so it's obvious at a glance which ones are still unsupported? I don't think there are very many left.

Makes sense to me, done

aside: Thanks, clippy... (best to review with whitespace ignored)

This is actually incorrect. It's perfectly legal to (attempt to) convert any Variant::Binary (plus maybe also Variant::String and Variant::Uuid) value to fixed-len binary -- including length 16. The conversion just might fail if the length is wrong.

We really need a UUID extension type check to know whether the caller intended to work with UUID or binary values.

I agree, which is why I removed the branch that errored on FixedSizeBinary(size). But this issue should be handled in a separate PR as it needs to change the impl the FixedSizeBinary. I'll add a TODO here.