[ntuple] Add some internal utilities to the storage layer #19904
Conversation
Test Results 22 files 22 suites 3d 19h 53m 2s ⏱️ For more details on these failures, see this check. Results for commit 722b86a. ♻️ This comment has been updated with latest results. |
Not sure...the reason I need it in the full PR is to create the AttrSetWriter which then uses it create a PageSinkFile using it. Even if we wanted to store the key in the root directory we'd still need the underlying TFile for it... |
silverweed
force-pushed
the
ntuple_attr_2
branch
2 times, most recently
from
3675b97 to
8cc71a8
Compare
There was a problem hiding this comment.
Some comments inline. I'm not actually sure it helps to review internal methods and additions to the RPageStorage abstraction layer without seeing the immediate need. Maybe one has to look at the final PR all the time, but that is quite some mental overhead. It feels like we are now vertically building the implementation, instead of in logical chunks...
silverweed
force-pushed
the
ntuple_attr_2
branch
3 times, most recently
from
f5e1b8c to
79698ad
Compare
I split it like this because this way it could be reviewed in parallel with the other PR rather than being blocked by it, but if you prefer I can also close this, wait for the other to be merged and then put this change together with the parts that use it (probably doing one PR for attribute reading, one for attribute writing and one for merging) |
| virtual std::vector<std::unique_ptr<ROOT::Internal::RCluster>> | ||
| LoadClusters(std::span<ROOT::Internal::RCluster::RKey> clusterKeys) = 0; | ||
|
|
||
| virtual RMiniFileReader *GetUnderlyingReader() { return nullptr; } |
There was a problem hiding this comment.
Do we need that? Aren't we using the reader only within ReadAttrSet?
There was a problem hiding this comment.
It is also used by the RNTupleMerger, since we need to fetch the Anchor when opening the attribute sets during merging (see https://github.com/root-project/root/pull/19153/files#diff-3fe10805b28dcc612504e58728446c32a3f70f4e42b5a6d62e46d07facd46766R127)
Will be used by the RNTupleAttributes.
Also update the outdated comment about GetNEntries()
silverweed
force-pushed
the
ntuple_attr_2
branch
from
79698ad to
722b86a
Compare
3 participants
Add a bunch of functions to poke through the abstraction of the storage classes (specifically, to get the underlying TDirectory and RMiniFileReader). These functionalities (all Internal) will be needed by the RNTuple Attributes (see the full PR for details)
Checklist: