[Variant] Fix cast logic for Variant to Arrow for DataType::Null

[klion26]

Which issue does this PR close?

• Closes Variant to NullType conversion ignores strict casting #8810 .

What changes are included in this PR?

Fix the logic for VariantToNullArrowRowBuilder so that it respects the cast option

Are these changes tested?

added test

Are there any user-facing changes?

Noe

[klion26]

@scovich @liamzwbao Please help review this when you're free ,thanks.

[klion26]

Choose to return err because Arrays of type Null cannot contain a null bitmask. If we return NullArray::new(valid_value_count), then the length of the result may not be the same as the input(the other types will add None in the result)

[scovich]

Agree we can't add a NULL mask to a NullArray. But I thought the idea was to let the normal strict-mode checking either error out or call (our fake) append_null that actually does the same thing as append_value (namely nothing). No null masks in sight.

[scovich]

The comment in the issue about counting appended values/nulls was kind of unrelated: technically there's no guarantee the number of appends matches the initial capacity estimate, which in arrow API is only an estimate. So we should ideally have both append_null and append_value increment some counter, whose final value dictates the size of the final array we create.

[klion26]

@scovich Thanks for the review and sorry for the late reply; I had to handle some unforeseen circumstances last week.

I've addressed the comments in the new commit. Please take another look.

In the last commit, I assumed that before append_null here, it can't be accepted as the target type is Null(we can't distinguish the target type and the missing element), but it seems append_null when value.as_null.is_none() in safe mode is fine(we may don't need to distinguish the two types, and there is no null mask in NullArray).

[klion26]

We use arrow::compute::CastOptions in variant_get and crate::CastOptions in cast_to_variant_with_options, not sure if we need to unify these in the future.

[liamzwbao]

I remember we had similar discussion before. I vote for unifying them, and we don't really need the format_options in the arrow::compute::CastOptions

[klion26]

Will file an issue to track this
Filed an issue #8873

[scovich]

define_variant_to_primitive_builder! macro handles all this already?
Why not use it?

|value| matches!(value, Variant::Null).then_some(0)

and then

fn append_value(&mut self, _dummy: i32) {
    self.item_count += 1;
}
fn append_null(&mut self) {
    self.item_count += 1;
}

(no need to capture the cast options, the macro handles that part)

[klion26]

Yes, it can reuse the define_variant_to_primitive_builder! macro, updated.

[klion26]

@scovich I've updated the code. Please take another look, thanks

[scovich]

LGTM!

[scovich]

tiny nit: Consider #[derive(Default)] and FakeNullBuilder::default() at L723 below?

[klion26]

Thanks for the suggestion, it's more elegant, added in the new commit

[liamzwbao]

lgtm! Thanks for the fix

[liamzwbao]

I remember we had similar discussion before. I vote for unifying them, and we don't really need the format_options in the arrow::compute::CastOptions

[klion26]

@alamb Please take a final review when you're free, thanks.

[alamb]

Thanks -- this looks good to me