Add UnionArray::into_parts
#5585
Conversation
arrow-array/src/array/union_array.rs
Outdated
| #[allow(clippy::type_complexity)] | ||
| pub fn into_parts( | ||
| self, | ||
| ) -> ( | ||
| DataType, | ||
| ScalarBuffer<i8>, | ||
| Option<ScalarBuffer<i32>>, | ||
| Vec<Option<ArrayRef>>, | ||
| ) { | ||
| let Self { | ||
| data_type, | ||
| type_ids, | ||
| offsets, | ||
| fields, | ||
| } = self; | ||
| (data_type, type_ids, offsets, fields) | ||
| } |
There was a problem hiding this comment.
It seems PrimitiveArray is the only other array type that returns a DataType as part of its into_parts()
Maybe just return the fields, akin to how StructArray does it? (Though unsure about the Sparse/Dense enum)
There was a problem hiding this comment.
Good suggestion. In that case I think we should also return UnionMode (instead of users having to check if the offsets buffer for Some or None to figure that out):
(UnionFields, UnionMode, ScalarBuffer<i8>, Option<ScalarBuffer<i32>>, Vec<Option<ArrayRef>>)
arrow-array/src/array/union_array.rs
Outdated
| DataType, | ||
| ScalarBuffer<i8>, | ||
| Option<ScalarBuffer<i32>>, | ||
| Vec<Option<ArrayRef>>, |
There was a problem hiding this comment.
This is exposing what is effectively an implementation detail of UnionArray, that it uses a sparse vec to encode type ids. We may want to revisit this in future, e.g. to better handle large type ids
|
I'm honestly a little torn on this as UnionArray is a bit of an oddity for two reasons:
I think it would be good to understand the motivating use-case for this, as AFAICT the output of into_parts wouldn't be useful for reconstructing a UnionArray |
My use-case is zero-copy conversion to a different union array type (test example here). I'm not using it to reconstruct an |
arrow-array/src/array/union_array.rs
Outdated
| field | ||
| .into_iter() | ||
| .zip(fields.into_iter().flatten()) | ||
| .collect(), |
There was a problem hiding this comment.
I don't think this is correct if the type ids in the signature are out of order
arrow-array/src/array/union_array.rs
Outdated
| assert_eq!(fields.len(), 2); | ||
|
|
||
| let result = UnionArray::try_new( | ||
| &[0, 1], |
There was a problem hiding this comment.
Might be worth showing this works with type ids of [6, 2] or something
arrow-array/src/array/union_array.rs
Outdated
| &[0, 1], | ||
| type_ids.into_inner(), | ||
| offsets.map(ScalarBuffer::into_inner), | ||
| field |
There was a problem hiding this comment.
I think this flattening should probably be an implementation detail of into_parts
There was a problem hiding this comment.
Maybe the fields argument of into_parts could be changed to impl Iterator<Item = (i8, ArrayRef)> to hide implementation details?
There was a problem hiding this comment.
I think it should probably reconstruct the arrays in the order expected by the constructor and return these as a Vec
There was a problem hiding this comment.
Makes sense. The constructor expects the order (of the child arrays) to match the provided fields ids, however, if unknown, one could get these type_ids from the UnionFields iterator, which has no order guarantee:
arrow-rs/arrow-schema/src/fields.rs
Lines 422 to 427 in 5a0baf1
|
I took the liberty of removing UnionMode as it is implied by the presence or abscence of the offsets I also filed #5613 to track cleaning up these constructors |
Rationale for this change
Most arrays have an
into_partsmethod to destruct the array into its constituent parts: https://docs.rs/arrow-array/latest/arrow_array/?search=into_partsWhat changes are included in this PR?
This adds that
into_partsmethod toUnionArray.Are there any user-facing changes?
arrow_array::array::UnionArray::into_parts