[ntuple] Attributes: add support for writing#21289
[ntuple] Attributes: add support for writing#21289silverweed wants to merge 8 commits intoroot-project:masterfrom
Conversation
Test Results 21 files 21 suites 3d 1h 57m 5s ⏱️ Results for commit 7ae91e3. ♻️ This comment has been updated with latest results. |
silverweed
force-pushed
the
ntuple_attr_write
branch
4 times, most recently
from
16e0a45 to
9b598df
Compare
silverweed
force-pushed
the
ntuple_attr_write
branch
from
9b598df to
505da84
Compare
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
A first review... I think one high-level decision to make is where to build RNTupleAttrSetDescriptor. RNTupleFileWriter feels like the wrong place to do this.
| inline const std::uint16_t kSchemaVersionMajor = 1; | ||
| inline const std::uint16_t kSchemaVersionMinor = 0; |
There was a problem hiding this comment.
Should these move into the RNTuple class where we have the other constants such as kVersionEpoch?
There was a problem hiding this comment.
Possibly; though these only concern attributes so it's not necessarily interesting for users of the RNTuple class. It's a matter of deciding whether we want to centralize these binary specs-related constants or not.
I have no strong opinion either way.
There was a problem hiding this comment.
Thinking again about this, I'd not put these in RNTuple as that's only one specific anchor, whereas these constants are anchor-agnostic.
If anything they might live in the Serializer, but I'd rather keep them close to the other related constants (rangeStartFieldName etc).
Probably in a separate header that is only included by some cxx file and not directly exposed to users.
There was a problem hiding this comment.
AFAICT we are already using the constants in RNTuple to initialize the RNTupleDescriptorBuilder...
There was a problem hiding this comment.
That's true, but I'm not sure that was the best decision on our side. In retrospect, I'd probably have put them in some dedicated namespace (although I see the argument of using the RNTuple name to namespace them)...
There was a problem hiding this comment.
If you're ok with it I would for now keep them somewhere Internal so we can change our mind later. It's not a consequential decision anyway. They can go either in the RNTupleAttrWriting header or in the RNTupleSerializer probably.
|
Thanks for the thorough review @hahnjo! For the descriptor building I agree it feels weird to have it in the Writer; let's talk about it offline. |
silverweed
force-pushed
the
ntuple_attr_write
branch
8 times, most recently
from
2d7c429 to
b20c52b
Compare
silverweed
force-pushed
the
ntuple_attr_write
branch
from
b20c52b to
0e6734f
Compare
silverweed
force-pushed
the
ntuple_attr_write
branch
from
0e6734f to
e5486e6
Compare
silverweed
force-pushed
the
ntuple_attr_write
branch
from
e5486e6 to
7ae91e3
Compare
3 participants
This Pull request:
adds the
RNTupleAttrSetWriterclass and initial support for writing attributes. This involves touching, among other classes, the RNTupleWriter, RNTupleFillContext, RPageSink and RMiniFile.Original reference PR (now quite outdated): #19153
Checklist: