-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ntuple] RMiniFile: write StreamerInfo with the correct compression #17565
base: master
Are you sure you want to change the base?
[ntuple] RMiniFile: write StreamerInfo with the correct compression #17565
Conversation
For context, the same PR (in spirit) was applied to |
@@ -1220,7 +1220,7 @@ void ROOT::Experimental::Internal::RNTupleFileWriter::Commit() | |||
|
|||
WriteTFileNTupleKey(); | |||
WriteTFileKeysList(); | |||
WriteTFileStreamerInfo(); | |||
WriteTFileStreamerInfo(compression); |
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.
It seems odd that the compression argument is only needed here. What compression setting is used for the list of keys, free list and for the RNTuple
key?
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.
I am under the impression that they are always uncompressed, (the first two are uncompressed even in regular TFiles as far as I saw).
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.
@pcanal let me know if you have other questions/comments or if this thread is resolved
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.
I am under the impression that they are always uncompressed,
What lead you to this conclusion?
the first two are uncompressed even in regular TFiles as far as I saw
Indeed the list of keys is not compressed (not sure why though) but I don't see a mechanism why the RNTuple
anchor would not be compressed (besides maybe the compression failing to save space)
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.
What lead you to this conclusion?
Reading the code in RMiniFile the only time we call Zip
is in the WriteStreamerInfo function. Also, all RNTuple files I've inspected so far had both sections uncompressed, regardless of whether they have been written through RMiniFile or TFile.
but I don't see a mechanism why the RNTuple anchor would not be compressed
I suppose it may be compressed if we manually wrote a RNTuple anchor through the TFile API, but if we pass through RMiniFile reader it will always be uncompressed (which is a behavior we may or may not want to change - though not in this PR I'd argue)
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.
Also, all RNTuple files I've inspected so far had both sections uncompressed, regardless of whether they have been written through RMiniFile or TFile.
That was really the question at hand. Is it compressed in TFile
? It should have been but if not, why/how is it not compressed.
(which is a behavior we may or may not want to change - though not in this PR I'd argue)
As we are touching that code, I would include the change also in this PR. The new code is:
WriteTFileNTupleKey();
WriteTFileKeysList();
WriteTFileStreamerInfo(compression);
with `compression being a new variable. This leaves 'ambiguous' whether the other two are 'not compressed' or 'compressed with an arbitrary value ignoring the user specified compression for undisclosed reasons'.
At the very least we would have:
WriteTFileNTupleKey(); // Not compressed yet
WriteTFileKeysList(); // Intentionally never compressed.
WriteTFileStreamerInfo(compression);
or better
WriteTFileNTupleKey(compress /* or 0 if one wants to keep the current erroneous behavior */ );
WriteTFileKeysList(); // Intentionally never compressed.
WriteTFileStreamerInfo(compression);
Test Results 18 files 18 suites 5d 1h 40m 45s ⏱️ For more details on these failures, see this check. Results for commit 4aeae98. |
So our first |
Currently RMiniFile doesn't honor the compression settings set in the RNTupleWriteOptions when writing out the StreamerInfo, but instead it always ZLIB-compresses it.
With this change, it will use the same compression as it was specified in the options (or 505 if unspecified).
This makes the behavior more consistent with the TFile, where the TFile compression determines also the StreamerInfo's compression (whereas now RNTuple may export a compressed StreamerInfo even if the TFile's compression is 0).
Checklist: