Skip to content

[ntuple] Add some Internal helpers needed for Attributes #19060

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

Closed

Conversation

silverweed
Copy link
Contributor

  • const version of GetProjectedFieldsOfModel()
  • RPageSink::GetUnderlyingDirectory()

These functionalities are needed in the RNTuple Attribute prototype (they are both Internal so there is no long-term implication of adding them).

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

- const version of GetProjectedFieldsOfModel()
- RPageSink::GetUnderlyingDirectory()
@silverweed silverweed requested review from hahnjo and enirolf June 17, 2025 09:10
@silverweed silverweed self-assigned this Jun 17, 2025
@silverweed silverweed requested a review from jblomer as a code owner June 17, 2025 09:10
@@ -313,6 +313,8 @@ public:

virtual ROOT::NTupleSize_t GetNEntries() const = 0;

virtual TDirectory *GetUnderlyingDirectory() const { return nullptr; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the abstraction of RPageSink quite heavily, since not all sinks are a file and even of those not all have a TDirectory. I'm not sure this makes sense to ask a TDirectory from all page sinks...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the wanted functionality is to open a new page sink that ends up in the same storage? Maybe we can have an interface virtual std::unique_ptr<RPageSink> CreateAtSameLocation(std::string_view ntupleName) that throws if not implemented by a sink, or if the sink is unable to provide that functionality (simple RMiniFile, for the moment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but for the prototype Attributes only work with TFile-based RNTuples and breaking the abstraction makes it much easier to deal with...at this point maybe the prototype should not be merged in master yet, until there is a more finalized design?
Either that or we merge it and later change it (making sure everything is in Internal/Experimental so we can do that easily)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the prototype itself, I think we cannot merge until we are sure the on-disk format is right because it requires bumping (at least) the minor version. In the past, we did merge interface extensions to make work-in-progress prototypes easier (my work on distributed parallel writing, the RNTupleProcessor while it was / is still evolving), but I think we should still strive to get the interfaces right (my 2 cents at least).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the binary format...we definitely cannot merge that until we're sure.

Copy link

Test Results

    18 files      18 suites   3d 17h 10m 29s ⏱️
 2 856 tests  2 856 ✅ 0 💤 0 ❌
49 966 runs  49 966 ✅ 0 💤 0 ❌

Results for commit 34a5563.

@silverweed silverweed closed this Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants