-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntuple] Attributes: update specs and add de/serialization #19894
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 22 files 22 suites 3d 21h 51m 39s ⏱️ For more details on these failures, see this check. Results for commit bb2d966. ♻️ This comment has been updated with latest results. |
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.
In principle looks good to me. Some comments.
b0449c0
to
48b72ff
Compare
I updated the PR and uniformed the |
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.
Very nice! Some minor comments.
Since this is changing the binary format, let's perhaps get a second approval.
48b72ff
to
07b8b20
Compare
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.
some comments on the spec additions and the iterator implementations
07b8b20
to
6c3a26f
Compare
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 in principle! Just some additional suggestions to the spec to improve readability and consistency.
6c3a26f
to
024401a
Compare
024401a
to
3a7b596
Compare
3a7b596
to
135bbfa
Compare
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.
Looks good from my side! A second approval would be good.
323e8bb
to
2940de0
Compare
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.
Thanks for reorganizing the spec additions, I (personally) find them easier to follow this way. It's still mentioning "model" in some places (notably in the _userModel
field name), not sure if we want to fully remove it...
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 think we need one more change with this PR: the RNTuple anchor needs to update the schema version to match the new specification.
I'll remove it from the leftover places; I guess at this point I should also rename the field to |
Or actually |
3a8b73c
to
da574f2
Compare
Added Attribute Sets
da574f2
to
2b715cd
Compare
2b715cd
to
bb2d966
Compare
First PR of a series to merge the RNTuple Attributes into master. The final result will be this, although the commits will be reorganized to be more coherent and reviewable.
This first PR updates the binary format specification (introducing a new minor version) and updates the Serializer and Descriptor code to match. This is backward-compatible and no Attribute can be written yet since the writer API will be introduced later.
Checklist: