[ntuple] Register post-read callbacks for #pragma read rules on user-defined classes
#11944
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce `RFieldBase::AddReadCallbacksFromIORules()`. This function registers a post-read callback for each of the given `ROOT::TSchemaRule`s.
I/O customization rules referencing non-transient members are ignored for now. Such rules shall trigger a warning, e.g. ``` 210: Warning in <[ROOT.NTuple] Warning /home/jalopezg/CERN/repos/root/tree/ntuple/v7/src/RField.cxx:931 in ROOT::Experimental::RClassField::RClassField(std::string_view, std::string_view, TClass*)::<lambda(const ROOT::TSchemaRule*)>>: ignoring I/O customization rule with non-transient member: a ```
|
Starting build on |
jblomer
requested changes
Dec 21, 2022
Contributor
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
Very nice! I think we should be able to check the class version as well. We should store the class version on disk and compare it with the rule version.
|
Starting build on |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 298 more Failing tests:
|
|
Build failed on windows10/cxx14. |
|
Build failed on mac12/noimt. Warnings:
And 285 more Failing tests:
|
|
Build failed on ROOT-debian10-i386/soversion. Warnings:
And 220 more |
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
And 286 more Failing tests:
|
|
Build failed on mac11/cxx14. Warnings:
And 286 more Failing tests:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Warnings:
And 367 more Failing tests:
|
Cache the C++ type version for the field as specified in the `RNTupleDescriptor` during a `ConnectPageSource()` call. The type version is made available via `GetOnDiskTypeVersion()`.
- Oftentimes, read callback registration depends on information that is only available after the field is connected to a page source, e.g. the on-disk C++ type version. Therefore, this commit introduces `RFieldBase::RegisterReadCallbacks()` that is called at the end of `ConnectPageSource()` to register post-read callbacks as appropriate. The function can be overridden by derived classes. - Update `RNTupleView` constructor: check if field has callbacks after the `ConnectPageSource()` call.
|
Starting build on |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 60 more Failing tests:
|
|
Build failed on mac12/noimt. Warnings:
And 53 more Failing tests:
|
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
And 54 more Failing tests:
|
|
Build failed on mac11/cxx14. Errors:
Warnings:
And 54 more Failing tests:
And 7 more |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Warnings:
And 130 more Failing tests:
And 10 more |
|
Starting build on |
Contributor
Author
Fair enough (and... done!). |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Warnings:
And 130 more Failing tests:
And 7 more |
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
And 54 more Failing tests:
|
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 60 more Failing tests:
|
|
Build failed on mac12/noimt. Warnings:
And 53 more Failing tests:
|
|
Build failed on mac11/cxx14. Errors:
Warnings:
And 54 more Failing tests:
And 7 more |
jblomer
requested changes
Jan 9, 2023
Contributor
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
Cool, nice work!
Some minor comments and then it's ready to get merged. Just wondering, the unit tests should print warnings, which we may need to add to the list of expected warnings.
Co-authored-by: Jakob Blomer <jblomer@cern.ch>
|
Starting build on |
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
|
|
Build failed on mac11/cxx14. |
jblomer
approved these changes
Jan 11, 2023
Co-authored-by: Jakob Blomer <jblomer@cern.ch>
jalopezg-git
force-pushed
the
ntuple-enablereadrules
branch
from
f2e10f8 to
6da2d26
Compare
|
Starting build on |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request #11731 introduced internal support for per field post-read callbacks. This follow-up pull request registers a callback for each
#pragma readrule on user-defined classes.Currently,
#pragma readrules referencing non-transient members as atargetare intentionally disallowed -- a warning is logged in that case. Only target class version is checked, i.e. checksum is ignored.Raw read rules are not (and will likely not be) supported, as they take a
TBuffer &.Changes or fixes:
RFieldBase::AddReadCallbacksFromIORules(). This function registers a post-read callback for each of the givenROOT::TSchemaRules.RFieldBase::ConnectPageSource(): cache C++ type version as stored in the RNTupleDescriptor. This information can be accessed viaGetOnDiskTypeVersion().RFieldBase::RegisterReadCallbacks()is called as part ofConnectPageSource(). This function can be overridden; in particular, derived classes can make use of the on-disk type version to enable/disable read rules.Checklist:
This PR partially takes care of #10019.