Add support for FixedSizeList to variant_to_arrow#9663
Add support for FixedSizeList to variant_to_arrow#9663rishvin wants to merge 6 commits intoapache:mainfrom
Conversation
|
@sdf-jkl could you help review this PR? |
| match value { | ||
| Variant::List(list) => { |
There was a problem hiding this comment.
Can reuse variant_cast_with_options
| match value { | |
| Variant::List(list) => { | |
| match variant_cast_with_options(value, self.cast_options, Variant::as_list) { |
| pub(crate) struct VariantToFixedSizeListArrowRowBuilder<'a> { | ||
| field: FieldRef, | ||
| list_size: i32, | ||
| element_builder: Box<VariantToShreddedVariantRowBuilder<'a>>, |
There was a problem hiding this comment.
Should reuse the new ListElementBuilder. Otherwise, variant_to_arrow on FixedSizeArray always returns a shredded array. The same error for other List builders was fixed here - #9631
| element_builder: Box<VariantToShreddedVariantRowBuilder<'a>>, | |
| element_builder: ListElementBuilder<'a>, |
| element_data_type: &'a DataType, | ||
| list_size: i32, | ||
| cast_options: &'a CastOptions, | ||
| capacity: usize, |
There was a problem hiding this comment.
Add a shredded param like the List builder above.
| capacity: usize, | |
| capacity: usize, | |
| shredded: bool, |
Can reuse the rest of the logic from VariantToListArrowRowBuilder
| #[test] | ||
| fn test_variant_get_fixed_size_list_not_implemented() { | ||
| let string_array: ArrayRef = Arc::new(StringArray::from(vec!["[1, 2]", "\"not a list\""])); | ||
| fn test_variant_get_fixed_size_list_with_safe_option() { |
There was a problem hiding this comment.
Once variant_to_arrow returns non-shredded arrays the whole test could be moved to test_variant_get_list_like_safe_cast test above.
| fn test_variant_get_fixed_size_list_with_safe_option() { |
| } | ||
|
|
||
| #[test] | ||
| fn test_variant_get_fixed_size_list_wrong_size() { |
There was a problem hiding this comment.
This test is wrong. We're trying to follow arrow-cast semantics for casts. This basically is a cast from List to a FixedSizeList(_, 2). We should only return Err if safe flag is false.
A snippet from arrow-cast:
arrow-rs/arrow-cast/src/cast/mod.rs
Lines 629 to 630 in 43d984e
| } | ||
|
|
||
| #[test] | ||
| fn test_array_shredding_as_fixed_size_list_wrong_size() { |
There was a problem hiding this comment.
This test should follow the same cast logic as variant_to_arrow. Safe cast -> Null, not Err.
We'd need to wire in the change to VariantToShreddedArrayVariantRowBuilder::append_value Variant::List match arm.
|
Thanks @sdf-jkl for reviewing changes. I have addressed the comments, could you re-review ? |
3 participants
variant_to_arrowFixedSizeListtype support #9531Rationale for this change
Add support for
FixedSizeListwhen invokingvariant_to_arrow.What changes are included in this PR?
VariantToFixedSizeListArrowRowBuilder.FixedSizeList.Are these changes tested?
By adding few test cases.
Are there any user-facing changes?
N/A.