Repository navigation
GH-22342: [Format][Documentation] Clarify Union type ids - #50839
Conversation
|
|
|
Formatting follow-up:
|
|
Nice focused improvement, LGTM! |
|
|
|
@paleolimbot @tustvold @alamb What do you think about these clarifications in the format spec? |
|
I think the change makes sense, an example might clear up any remaining ambiguity |
| Each child type in a union has a type id (an 8-bit signed integer) | ||
| that identifies it in the types buffer. By default, these type ids | ||
| correspond directly to the child array indices (0, 1, ...), but an optional | ||
| ``typeIds`` metadata array can provide an explicit mapping from child array |
There was a problem hiding this comment.
It isn't clear from this how they actually do this, I think an example would make it clear.
There was a problem hiding this comment.
Also possibly calling it just typeIds metadata (the term "array" here I believe is talking about a different kind of array than it is later in the paragraph)
paleolimbot
left a comment
There was a problem hiding this comment.
Definite improvement! I found this confusing when I implemented it. I don't believe that creating these types of unions is common (and agreed that an example would help).
For what it's worth, GeoArrow does use these and does use meaningful type IDs (although they aren't widely used compared to the other types in the spec). https://geoarrow.org/format.html#geometrycollection
| Each child type in a union has a type id (an 8-bit signed integer) | ||
| that identifies it in the types buffer. By default, these type ids | ||
| correspond directly to the child array indices (0, 1, ...), but an optional | ||
| ``typeIds`` metadata array can provide an explicit mapping from child array |
There was a problem hiding this comment.
Also possibly calling it just typeIds metadata (the term "array" here I believe is talking about a different kind of array than it is later in the paragraph)
alamb
left a comment
There was a problem hiding this comment.
Thanks @pitrou -- this seems like an improvement to me
I think @tustvold and @paleolimbot 's suggestions are good ones too, but I also think this could be merged as is as it is an improvement over the current version in my mind
|
I've applied some minor modifications to address some of the comments, in the interest of moving this forward. I think adding an example can be done in a followup task if someone is motivated to do so. |
|
I'll wait for CI and then merge. |
Rationale for this change
The Union layout documentation does not explain that the type IDs stored in the types buffer can differ from the child array indices, or how the optional
typeIdsmetadata maps children to physical type IDs.What changes are included in this PR?
typeIdsmetadata.Fixes #22342.
Are these changes tested?
git diff --checkpasses and the RST change was reviewed againstSchema.fbsand the existing Union layout documentation. A local Sphinx/RST checker is not installed in this environment.Are there any user-facing changes?
No.