-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARROW-13055: [Doc] Create canonical extension types document #14167
ARROW-13055: [Doc] Create canonical extension types document #14167
Conversation
@wjones127 Would you like to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
A few things:
-
Weston had some comments in the vote thread, will those be integrated?
This is maybe implied but I would add that
modification of extension types must also require a vote and should be
backwards compatible. Furthermore, extension types (particularly
those with extensive parameterization/serialization should discuss how
future additions would be made. For example, if serialized via JSON
than readers should tolerate (and ignore) unexpected keys. -
Maybe add 'canonical extension type' to Glossary.rst as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. Once we have our first canonical type, hopefully we will have a model proposal on the ML we can link to. It would be nice, for example, to show what kind of detail the semantics should be described in.
2) Its parameters, if any, *must* be described in the proposal. | ||
|
||
3) Its serialization *must* be described in the proposal and should | ||
not require unduly work or unusual software dependencies (for example, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "unduly work"? Is that in terms of readability to reviewers? Or in terms of computation time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in terms of implementation effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this more explicit, though ideally we should not deviate too much from what has been voted on on the ML.
4578e16
to
6c60780
Compare
@lidavidm @wjones127 I believe I addressed your comments, could you take another look? |
Benchmark runs are scheduled for baseline = 2577ac1 and contender = 2629f20. 2629f20 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…14167) Vote result at https://lists.apache.org/thread/sxd5fhc42hb6svs79t3fd79gkqj83pfh Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
…14167) Vote result at https://lists.apache.org/thread/sxd5fhc42hb6svs79t3fd79gkqj83pfh Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Vote result at https://lists.apache.org/thread/sxd5fhc42hb6svs79t3fd79gkqj83pfh