[Variant] Fix the variant shred logic

[klion26]

Which issue does this PR close?

• Closes Fix the shred logic in parquet-variant #10145.

What changes are included in this PR?

• Seperate the logic for variant_shred and variant_get by introduce shred flag for row builder
• Move some logic in Variant::as_xxx to type_conversion, and these moved code will be used when variant_get, variant_shred will use Variant::as_xxx
• After the change, when shredding a variant, all Variant::as_xx() is identity function now, Variant::Int8 can only be treated as int8, but not int16/int32/int64, etc.
• Removed the Variant::as_f16
• Add a test to cover that shred can/can't be shredded to some datatype in test_variant_type_shredded_correctly

Are these changes tested?

Yes, added some tests to cover the logic

Are there any user-facing changes?

Yes, some Variant::as_xx logic have been changed.

[klion26]

@scovich @sdf-jkl Please help review this when you're free, thanks.

[klion26]

These code are mainly moved from Variant.rs, the change here was adding the parameter shred for from_variant and variant_to_unscaled_decimal.

[sdf-jkl]

Hey @klion26 I took a look.

Overall I like the approach but I think we can improve it.

To follow more closely the existing split between arrow-array and arrow-cast we can only keep identity accessors for just the specific type without widening/narrowing. (i.e. Variant::as_i32 only if the Variant is i32) (it'd also address the lossy cast comment below)

And as for switching between shred_variant and variant_get cast semantics, we could pass them through variant_cast_with_options and add an argument for cast semantics we use { shred, get }

[sdf-jkl]

I originally thought it's a wrong idea to decouple cast from parquet-variant crate, but now I think it's very similar to how arrow-array and arrow-cast are decoupled.

[sdf-jkl]

Suggested change

	false,
	true,

if we keep false here it preserves the old too lenient cast semantics.

The branch explicitly checks if shredded is true.

Can check with (doesn't pass):

    // `VariantToListArrowRowBuilder::try_new` hardcodes `shred = false` for its element
    // builder (see variant_to_arrow.rs). The struct-field path threads `shred` correctly, so this
    // asymmetry is invisible without a nested-list test.
    #[test]
    fn test_array_shredding_does_not_cast_bool_element_to_int() {
        let input = build_variant_array(vec![VariantRow::List(vec![
            VariantValue::from(1i64),
            VariantValue::from(true),
            VariantValue::from(2i64),
        ])]);
        let list_schema = DataType::List(Arc::new(Field::new("item", DataType::Int64, true)));
        let result = shred_variant(&input, &list_schema).unwrap();
        assert_eq!(result.len(), 1);

        assert_list_structure_and_elements::<Int64Type, i32>(
            &result,
            1,
            &[0, 3],
            &[Some(3)],
            &[None],
            (
                // element 1 (the bool) must NOT be shredded into typed_value ...
                &[Some(1), None, Some(2)],
                // ... it must be preserved verbatim in the residual `value` instead.
                &[None, Some(Variant::BooleanTrue), None],
            ),
        );
    }

[klion26]

As, thanks for pointing this out, this was my bad, I used false as a placeholder, but did not change me all.

[sdf-jkl]

Suggested change

	false,
	true,

same as above

[sdf-jkl]

Should shred do lossy narrowing f64 to f32?

[klion26]

@sdf-jkl Thanks for the review, I've addressed the comments, please take another look when you're free.

For the identity get for Variant::as_xx, I have thought that, but keep it as now, because the underlying physical data is the same(don't do lossless conversion), but I'm open for this question.

For the switching between shred_variant and variant_get, I left the switch logic to the definition of the builder -- here we know how to do the conversion better, variant_cast_with_options seems like a wrapper of them, so I keep as it for now.

[klion26]

After another look of the code, I perfer the identity function for Variant::as_xx now, as some double can't convert to float even located in [f32::MIN, f32::MAX], this makes the semantics of Variant::as_f32 tricky(users may not know which double(f64) can or can't be convert to f32 easily), -- for shred, this may led to some double (in the same array) shred to f32 into typed_value column, and some double(which can't convert into f32 lossless) located into value column.

After changing to the identity function for Variant::as_xxx, there is a problem for Variant::as_u8/16/32/64, how do we want to handle this?
if we align the semantics for all of the Variant::as_xxx, maybe we can only return Some(value) for Variant::as_u8 when it is Variant::Int8 and None for the other variants, etc

fn as_u8(&self) -> Option<u8> {
   match *self {
     Variant::Int8(i) => i.try_into().ok(),
     _ => None,
   }
}

...

fn as_u64(&self) -> Option<u64> {
    match *self {
       Variant::Int64(i) => i.try_into().ok(),
      _ => None,
}

[sdf-jkl]

If we go identity functions for Variant::as_xx we can drop Variant::as_u8/16/32/64 from parquet-variant entirely and keep them only as cast semantics in parquet-variant-compute?

We don't shred to unsigned types and the only path to use it would be cast in variant_get, so parquet-variant can safely drop them.

[klion26]

I've update the code,

• changed the Variant::as_xx as identity function
• removed Variant::as_f16, -- will always return None for as_f16 if we keep it.

but don't remove the Variant::as_u8/16/32/64 for now, we use Variant::as_xx in the shred logic, but Variant::as_xx is not for shred use only, so I don't remove them for now, Variant::as_u8 returns Some(v) for Variant::Int8 but None for other variants. I can remove them if reached to a consensuse that we want to remove Variant::as_u8/16/32/64

[klion26]

Align the cast logic with arrow-cast for Time related variant will in a followup pr