-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntuple][rfile] Add support for RFile in RNTupleWriter #20909
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
Conversation
Test Results 22 files 22 suites 3d 17h 8m 47s ⏱️ For more details on these failures, see this check. Results for commit ff09606. ♻️ This comment has been updated with latest results. |
864f938 to
0f1b792
Compare
0f1b792 to
91bc815
Compare
hahnjo
left a comment
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 code-wise, I'll let Jakob approve the interface part (which I guess you already discussed offline)
jblomer
left a comment
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.
I think we should have a follow-up PR to rename the internal file structs in the mini file writer, e.g. RImplSimple, RImplTFile, RImplRFile... or something better. It just seems that currently the code overuses the word File.
91bc815 to
11610e1
Compare
|
Added a python test (and a small pythonization of the RFile-based Append) |
11610e1 to
fc1496e
Compare
Just for the time being, to avoid exposing experimental API as a public method of the RNTupleWriter, use a free friend function in the Experimental namespace. When we settle on the RFile/RNTuple writing semantics we will remove this function and replace it with a factory method.
|
Since the python test and pythonization are blocked by #21030, for this PR I'll only add the C++ part and will do the python side on a separate one. |
fc1496e to
ff09606
Compare
| operator bool() const { return fDirectory; } | ||
| }; | ||
|
|
||
| struct RFileRFile { |
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.
This class would benefit from a short introduction to its raison d'être as the name itself is not self explanatory.
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.
See #21100
|
This Pull request:
allows RMiniFileWriter to handle RFile and exposes an experimental way to create an RNTupleWriter using RFile.
Currently the semantics are the same as the
TDirectoryoverload: you pass in a reference toRFileand are responsible to keep the file alive until the RNTupleWriter is done.Main question to answer: is this the semantics we want? It seems to me that it's the most straightforward API in C++, but it requires some extra work in python to avoid use-after-free cases like:
This should be solveable by pythonizing
Appendto hold on to the file's proxy on the python side, but there might be additional complications I'm not considering. I'm open for discussion.Checklist: