-
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] Add RNTupleSingleProcessor
#17134
Conversation
Test Results 18 files 18 suites 4d 2h 33m 57s ⏱️ Results for commit 8c5a984. ♻️ This comment has been updated with latest results. |
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.
Very nice! I have a couple of minor remarks, but the code looks good already!
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! Although it would be nice to not treat empty data sets in a special way.
Indeed, this processor should also be able to handle empty ntuples. I have an implementation for this, but it required a bit more refactoring than I initially forsaw, so I propose to merge this PR as-is, and I will open a new one to address the empty ntuple case. |
The `RNTupleSingleProcessor` processes one single RNTuple, using the same interfaces as the `RNTupleChainProcessor` and `RNTupleJoinProcessor`. The point of adding this processor is that this way, the other processors can use a unified interface for loading entries, without having to know whether they are dealing with a single RNTuple or a composed one.
84d59d0
to
8c5a984
Compare
RNTupleBaseProcessor
RNTupleSingleProcessor
The
RNTupleSingleProcessor
processes one single RNTuple, using the same interfaces as theRNTupleChainProcessor
andRNTupleJoinProcessor
. The point of adding a base processor is that this way, the other processors can use a unified interface for loading entries, without having to know whether they are dealing with a single RNTuple or a composed one.This PR is part of a bigger set of (foreseen) changes, collected and tracked in #17132.