[ntuple] Attributes: add support for writing#21289
[ntuple] Attributes: add support for writing#21289silverweed wants to merge 6 commits intoroot-project:masterfrom
Conversation
Test Results 18 files 18 suites 2d 16h 49m 45s ⏱️ For more details on these failures, see this check. Results for commit 505da84. ♻️ 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.
| #ifndef ROOT7_RNTuple_Attributes | ||
| #define ROOT7_RNTuple_Attributes |
There was a problem hiding this comment.
A while ago, we started having one header per class. I think we should keep this pattern and split the header RNTupleAttributes.hxx proposed here
There was a problem hiding this comment.
We still have some exceptions where it makes sense (RNTupleDescriptor.hxx, RNTupleUtils.hxx, ...); I'm not sure that splitting this would bring much value - perhaps the splitting could be made at the reading vs writing level?
| inline const char *const kRangeStartName = "_rangeStart"; | ||
| inline const char *const kRangeLenName = "_rangeLen"; | ||
| inline const char *const kUserModelName = "_userModel"; | ||
|
|
||
| inline constexpr std::size_t kRangeStartIndex = 0; | ||
| inline constexpr std::size_t kRangeLenIndex = 1; | ||
| inline constexpr std::size_t kUserModelIndex = 2; |
There was a problem hiding this comment.
These are (currently) not used in the header and should probably be moved to the implementation file, where needed.
There was a problem hiding this comment.
Some of these will be used by tests in a later PR - should I move them now and then move them back later?
| 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.
| ROOT::TestSupport::CheckDiagsRAII diagsRaii; | ||
| diagsRaii.requiredDiag(kWarning, "ROOT.NTuple", "RNTuple Attributes are experimental", false); |
There was a problem hiding this comment.
I guess this should already be there in the previous commit?
There was a problem hiding this comment.
The warning only triggers when the attribute set is committed, so this wouldn't show up until Commit() is implemented
|
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
from
505da84 to
a24ec5f
Compare
silverweed
force-pushed
the
ntuple_attr_write
branch
from
a24ec5f to
a66217a
Compare
2 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: