[Variant] Add list support to unshred_variant#8514
Conversation
github-actions
bot
added
parquet
|
CC @alamb @liamzwbao |
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
A few notes for reviewers
|
|
||
| //! Module for unshredding VariantArray by folding typed_value columns back into the value column. | ||
|
|
||
| use crate::arrow_to_variant::ListLikeArray; |
There was a problem hiding this comment.
Thanks for the nice new interface, @liamzwbao !
| /// | ||
| /// [valid]: VariantMetadata#Validation | ||
| /// [Variant spec]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#value-data-for-array-basic_type3 | ||
| #[derive(Debug, Clone, PartialEq)] |
There was a problem hiding this comment.
We forgot to fix this PartialEq when we fixed the one for VariantObject.
Manual implementation below.
| true | ||
| self.iter() | ||
| .zip(other.iter()) | ||
| .all(|((name_a, value_a), (name_b, value_b))| name_a == name_b && value_a == value_b) |
There was a problem hiding this comment.
Opportunistic simplification.
| ($case_num:literal) => { | ||
| paste::paste! { | ||
| #[test] | ||
| fn [<test_variant_integration_case_ $case_num>]() { | ||
| all_cases()[$case_num - 1].run() | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Generates an error test case, where the expected result is an error message | ||
| ($case_num:literal, $expected_error:literal) => { |
There was a problem hiding this comment.
Opportunistic simplification: Instead of duplicating the macro definition, just use $(...)? syntax to capture (and respond to) an optional error message.
| list_builder.finish(); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder how we could test this reasonably 🤔 Maybe we can rework the tests in cast_to_variant to shred and then unshred an array and verify it survives round tripping 🤔
| ($case_num:literal) => { | ||
| paste::paste! { | ||
| #[test] | ||
| fn [<test_variant_integration_case_ $case_num>]() { | ||
| all_cases()[$case_num - 1].run() | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Generates an error test case, where the expected result is an error message | ||
| ($case_num:literal, $expected_error:literal) => { |
| variant_test_case!(1, "Unshredding not yet supported for type: List("); | ||
| variant_test_case!(2, "Unshredding not yet supported for type: List("); | ||
| variant_test_case!(1); | ||
| variant_test_case!(2); |
There was a problem hiding this comment.
Do I read this diff correctly that after this PR we handle all the test cases other than Decimal? If so, that is pretty rad 🤯
(btw I think the reference to https://github.com/apache/arrow-rs/issues/8329 above variant_test_case!(4); is old and can be removed )
There was a problem hiding this comment.
Do I read this diff correctly that after this PR we handle all the test cases other than Decimal?
I believe so!
the reference to
https://github.com/apache/arrow-rs/issues/8329abovevariant_test_case!(4);is old and can be removed
Good catch! Fixed in #8481, since it has nothing to do with variant arrays.
Perhaps you meant |
|
I think this one is now ready to rebase and merge -- I tried to do it mechanically but I hit a conflict. @scovich since you are more familiar with this code, could you rebase the PR so I can merge it? |
Done! |
|
Thanks @scovich |
2 participants
Which issue does this PR close?
List#8337Rationale for this change
Add a missing feature.
What changes are included in this PR?
Leveraging the recently added
ListLikeArraytrait, support all five list types when unshredding variant data.Are these changes tested?
Yes -- all the list-related variant shredding integration tests pass now.
Are there any user-facing changes?
No.