[Variant] Preserve Field throughout Variant shredding flow
#8673
Conversation
| pub fn with_column(mut self, field_name: &str, array: ArrayRef, nullable: bool) -> Self { | ||
| let field = Field::new(field_name, array.data_type().clone(), nullable); | ||
| self.fields.push(Arc::new(field)); | ||
| self.arrays.push(array); | ||
| self | ||
| } | ||
|
|
||
| pub fn with_field(mut self, field: FieldRef, array: ArrayRef) -> Self { | ||
| self.fields.push(field); | ||
| self.arrays.push(array); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't love the naming here
There was a problem hiding this comment.
This is a convenience method, right?
We already have the datatype (from the array), so maybe we just need pass an optional metadata?, making this similar to the Field constructor? Or, if that's too disruptive for exiting callers, with_column_and_metadata?
| index: usize, | ||
| ) -> Variant<'a, 'a> { | ||
| let data_type = typed_value.data_type(); | ||
| let (_typed_value_field, typed_value_column) = typed_value; |
There was a problem hiding this comment.
Now that we have the field information, we can check for extension types like: e21dc1b
| .typed_value_field() | ||
| .unwrap() | ||
| .1 |
There was a problem hiding this comment.
Just glancing at this code in isolation, I would guess that .0 is the field and .1 is the array?
But that requires knowing context (ie of this PR)
If this usage will show up often, is it worth returning a newtype instead of a tuple?
| let typed_value = if let Some(typed_value_array) = typed_value.clone() { | ||
| let field_ref = Arc::new(Field::new( | ||
| "typed_value", | ||
| typed_value_array.data_type().clone(), | ||
| true, | ||
| )); | ||
| builder = builder.with_field(field_ref.clone(), typed_value_array.clone()); | ||
|
|
||
| Some((field_ref, typed_value_array)) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
nit: this is just Option::map (no ? to complicate things)
| let typed_value = if let Some(typed_value_array) = typed_value.clone() { | ||
| let field_ref = Arc::new(Field::new( | ||
| "typed_value", | ||
| typed_value_array.data_type().clone(), | ||
| true, | ||
| )); | ||
| builder = builder.with_field(field_ref.clone(), typed_value_array.clone()); | ||
|
|
||
| Some((field_ref, typed_value_array)) |
There was a problem hiding this comment.
because of the dual typing (one in field and one in array), this code is technically no longer infallible -- the data types could disagree. Not sure the best way to address that issue as long as we're passing a full-blown field.
There was a problem hiding this comment.
Also, this logic is duplicated with the variant array constructor above.
I think one thing I'm debating is the coupling between the |
2 participants
Which issue does this PR close?
FixedSizeBinary(16)shredding #8665Rationale for this change
This PR preserves the
typed_value'sFieldmetadata. This way, we can check for extension types.