Add UnionFields (#3955) #3981
Add UnionFields (#3955) #3981
Conversation
github-actions
bot
added
arrow
| } | ||
| } | ||
|
|
||
| impl UnionFields { |
There was a problem hiding this comment.
I purposefully kept the size of this interface down as I don't feel we have very mature support for UnionArrays and therefore I didn't want to paint ourselves into a corner too much w.r.t what this abstraction looked like
There was a problem hiding this comment.
Can we please add documentation that explains:
- what type_ids are (a link to the spec / docs)
- has an example of how to construct these things
There was a problem hiding this comment.
Reduces the size of DataType from 56 bytes to 24 bytes
🎉
I think given that the size of DataType is an important metric, we should also add a test that verifies it doesn't change by accident -- like https://github.com/apache/arrow-datafusion/blob/8e125d2ecf242b4f4b81f06839900dbb2037cc2a/datafusion/common/src/scalar.rs#L3728-L3738
| } | ||
| } | ||
|
|
||
| impl UnionFields { |
There was a problem hiding this comment.
Can we please add documentation that explains:
- what type_ids are (a link to the spec / docs)
- has an example of how to construct these things
| } | ||
|
|
||
| /// Create a new [`UnionFields`] from a [`Fields`] and array of type_ids | ||
| pub fn new<F, T>(type_ids: T, fields: F) -> Self |
There was a problem hiding this comment.
Should this be fallible (to prevent repeated type ids, for example)?
There was a problem hiding this comment.
I honestly wasn't sure about this, the existing code doesn't handle this consistently. I'll file a ticket
Edit: #3982
| self.0.iter().map(|(id, f)| (*id, f)) | ||
| } | ||
|
|
||
| pub(crate) fn try_merge(&mut self, other: &Self) -> Result<(), ArrowError> { |
There was a problem hiding this comment.
Can you please add some docs about what this does?
There was a problem hiding this comment.
Sorry, should have called out in review, just moved some code here. Added docs in 07af4ff
Which issue does this PR close?
Part of #3955
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?