[Variant] make value mandatory field for VariantArray/ShreddingState#10318
[Variant] make value mandatory field for VariantArray/ShreddingState#10318sdf-jkl wants to merge 3 commits into
value mandatory field for VariantArray/ShreddingState#10318Conversation
github-actions
Bot
added
parquet
| pub(crate) fn from_parts( | ||
| metadata: ArrayRef, | ||
| value: Option<ArrayRef>, | ||
| value: ArrayRef, |
There was a problem hiding this comment.
What if we still keep the func signature, but None means perfect shredding? so that the caller doesn't need to construct the all empty value column
There was a problem hiding this comment.
I thought about it, but I think it makes it misleading for a user. Seeing optional value field may lead them to believe that it really is optional for a VariantArray.
There was a problem hiding this comment.
since this is pub crate I think it fine (and not a public API)
| /// Construct a perfectly shredded `VariantArray`: every value is in | ||
| /// `typed_value` and the required `value` column is all-null. | ||
| #[cfg(test)] | ||
| pub(crate) fn perfectly_shredded( |
There was a problem hiding this comment.
Seems this is for better construct test cases, not sure if this is the right place to put it.
| // Always prefer typed_value, if available | ||
| (Some(typed_value), value) if typed_value.is_valid(index) => { | ||
| Some(typed_value) if typed_value.is_valid(index) => { | ||
| typed_value_to_variant(typed_value, value, index) |
There was a problem hiding this comment.
Will moving the check in typed_value_to_variant here be better? so that we don't need to pass the value column when callingtyped_value_to_variant, the fun name seems to be for typed_value but we still need to pass the value in
…ed-variant-require-value-10306
| pub(crate) fn from_parts( | ||
| metadata: ArrayRef, | ||
| value: Option<ArrayRef>, | ||
| value: ArrayRef, |
There was a problem hiding this comment.
I thought about it, but I think it makes it misleading for a user. Seeing optional value field may lead them to believe that it really is optional for a VariantArray.
| /// Construct a perfectly shredded `VariantArray`: every value is in | ||
| /// `typed_value` and the required `value` column is all-null. | ||
| #[cfg(test)] | ||
| pub(crate) fn perfectly_shredded( |
|
@alamb please take a look |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
What happens after this PR if arrow-rs tries to read invalid variants (that have no value column)? Will it error?
If so it might be nice to make sure that we accept potentially malformed data (especially as older versions of arrow-rs would write it) but always write "correct" data (with a "values" column)
| pub(crate) fn from_parts( | ||
| metadata: ArrayRef, | ||
| value: Option<ArrayRef>, | ||
| value: ArrayRef, |
There was a problem hiding this comment.
since this is pub crate I think it fine (and not a public API)
4 participants
Which issue does this PR close?
Rationale for this change
arrow-rs doesn't follow the Variant spec by making
valuefield optional.What changes are included in this PR?
valuefield mandatory forVariantArrayandShreddingState.Are these changes tested?
Are there any user-facing changes?
valuefield is now mandatory for Variant