-
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 22 files 22 suites 3d 15h 11m 1s ⏱️ For more details on these failures, see this check. Results for commit 11610e1b. ♻️ 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.
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.
91bc815 to
11610e1
Compare
|
Added a python test (and a small pythonization of the RFile-based Append) |
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: