[Variant] Fix cast logic for Variant to Arrow for DataType::Null #8825
Conversation
klion26
left a comment
klion26
left a comment
There was a problem hiding this comment.
@scovich @liamzwbao Please help review this when you're free ,thanks.
| if let Some(_) = value.as_null() { | ||
| Ok(true) | ||
| } else { | ||
| // Null type only accepts nulls |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Fix the logic for
VariantToNullArrowRowBuilderso that it respects thecastoptionAre these changes tested?
added test
Are there any user-facing changes?
Noe