[Variant] Add variant to arrow for DataType::{Utf8, LargeUtf8} and DataType::Binary

[klion26]

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

• Closes [Variant] Add variant to arrow for DataType::{Utf8, LargeUtf8} #8767.

What changes are included in this PR?

• Add a struct VariantToUtf8ArrowRowBuilder<'a, OffsetSizeTrait>, a struct VariantToBinaryRowBuilder<'a>
• Add two enums Utf8(VariantToUtf8ArrowRowBuilder<'a, i32>) and LargeUtf8(VariantToUtf8ArrowRowBuilder<'a, i64>) for PrimitiveVariantToArrowRowBuilder
• Add tests to cover the added logic

Are these changes tested?

Added new tests

Are there any user-facing changes?

No public API changed

[klion26]

@mbrobbel Thanks for the quick review. As the primitive builder for VariantToArrow now only left Utf8 and Binary(all the other types have been implemented, list/object will be implemented in other places), I've appended binary case to this pr, sorry for any inconvenience this may cause. (pushed to the remote branch before I noticed the review)

Appended two commits

• one to enhance the test name for Utf8/LargeUtf8
• the other one wants to support the DataType::Binary

[mbrobbel]

Thanks @klion26, the additional changes look good to me too.

[liamzwbao]

I think we already have a binary builder here, how about reusing it? Perhaps just change this line to make it work

Plus, this builder is called PrimitiveVariantToArrowRowBuilder but String and binary are not primitive type. So I think we need either rename the builder or put them into a new builder.

[klion26]

IIUC, the logic of VariantToBinaryVariantArrowRowBuilder is different from the Binary here, the VariantToBinaryVariantArrowRowBuilder is for the raw Variant, and here we want to process for Variant::Binary. I tried reuse the VariantToBinaryVariantArrowRowBuilder, it will contains the meta and other things also, we only need the binary value here.

And for the builder, please correct me if I'm wrong, the primitive here is for variant, not for the DataTypes, we put Null/Boolean/ Uuid in here also.

[liamzwbao]

Agreed on the first one as we may not need metadata column if explicitly cast to Binary.

For the 2nd, I think the primitive is for arrow because we check is_primitive and will throw error if the request data type is not primitive here. Also based on the definition here, this builder is to convert variant values to primitive arrow. Given that, having Null/Boolean/Uuid in this builder makes the intent tricky. I’d prefer we rename the builder and update the docs as needed. We don't need a separate builder tho as we can reuse a lot of codes here.

For Variant primitive, we had a related discussion before. I think Parquet enforces primitive types at write time and unshred to ensure valid data. But for variant_get, it’s reasonable to allow casting to any valid Arrow datatype, even those without a direct Variant-primitive counterpart (e.g., Decimal256 or LargeUtf8).

WDYT? @alamb @scovich

[klion26]

I think the builder is transform some variant to arrow type, and the Variant has primitive/shortstring/object/list types, Here we want to handle the types for Variant::Primitive(include shortstring) types, in shred_variant::make_variant_to_shredded_variant_arrow_row_builder/variant_to_arrow#make_variant_to_arrow_row_builder we have other match arms(for objects/lists), the whole match arms will equal to all variant types.

For

check is_primitive and will throw error if the request data type is not primitive

I think this is for NotYetImplementation Error

And have an expert to confirm this is better.

[scovich]

I think we already have a binary builder here, how about reusing it? Perhaps just change this line to make it work

The VariantToBinaryVariantArrowRowBuilder converts shredded variant to unshredded (binary) variant.

Here, we need a builder that can extract Variant::Binary values into a BinaryArray, with nulls (or errors) wherever the variant was not (convertible to) binary.

I believe this PR correctly does the latter, but only for DataType::Binary. It leaves out DataType::BinaryView, DataType::LargeBinary and DataType::FixedSizeBinary.

The first three can all be covered by a single generic builder, by defining a new BinaryLikeArrayBuilder trait. It would be very similar to the StringLikeArrayBuilder trait that #8600 already added for the same reason.

It's debatable whether we actually need fixed-size binary support. If we did support it, it would need a custom builder whose extraction would presumably fail for any Variant::Binary with the wrong length?

[liamzwbao]

I think the confusion comes from variant_get and shred_variant sharing variant_to_arrow.

• variant_get should allow casting to any valid Arrow type.
• shred_variant should follow the shredding spec (only Parquet primitives + list/struct).

My advice:

• Add type checks in shred_variant to enforce the spec.
• Keep variant_to_arrow a pure converter (the inverse of arrow_to_variant).

Created an issue #8795 to keep track of.

Overall, no blockers from my side. I’ll submit a follow-up PR to fix the docs. It would also be great to add support for LargeBinary and BinaryView either in this PR or a follow-up.

[klion26]

Will add LargeBinary and BinaryView and update the pr.

[sdf-jkl]

Hey @klion26, haven't we already finished variant_to_arrow support for Utf8 here #8600?

[klion26]

@sdf-jkl Thanks for the remainder, ah, sorry for the conflict, I'll rebase on the main branch after #8600 merged, seems #8600 ready for merge.

[sdf-jkl]

@klion26 #8600 was merged yesterday