Skip to content
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] Make RNTupleChainProcessor composable #17393

Merged

Conversation

enirolf
Copy link
Contributor

@enirolf enirolf commented Jan 9, 2025

This PR introduces the possibility to create RNTupleChainProcessors from other processor objects. In turn, this makes it possible to, for example, create a chain of joined ntuples.

This PR is part of a bigger set of (foreseen) changes, collected and tracked in #17132.

@enirolf enirolf self-assigned this Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Test Results

    18 files      18 suites   4d 13h 53m 5s ⏱️
 2 690 tests  2 689 ✅ 0 💤 1 ❌
46 722 runs  46 721 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 3da39b3.

♻️ This comment has been updated with latest results.

@enirolf enirolf force-pushed the ntuple-processor-chain-composition branch 2 times, most recently from 8d818f5 to 75745f8 Compare January 14, 2025 16:14
@enirolf enirolf force-pushed the ntuple-processor-chain-composition branch 2 times, most recently from bf4b538 to 4b69b26 Compare January 22, 2025 10:30
@enirolf enirolf marked this pull request as ready for review January 22, 2025 10:33
@enirolf enirolf requested review from jblomer and couet as code owners January 22, 2025 10:33
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of, mostly minor, comments.
In general lgtm but there are some changes introduced in one commit that are changed by later commits, so it's a bit hard to review commit-wise.

tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Outdated Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
@enirolf enirolf force-pushed the ntuple-processor-chain-composition branch from 4b69b26 to b538fcc Compare January 28, 2025 16:05
@enirolf enirolf requested a review from silverweed January 28, 2025 16:05
@silverweed
Copy link
Contributor

lgtm but better wait for other people's approval as well

Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! I have some questions on details.

tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Outdated Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
Prevents the page sources corresponding to the processor to be openened
upon creation. Instead, defer opening them until the first `Advance`
call.
@enirolf enirolf force-pushed the ntuple-processor-chain-composition branch from b538fcc to e70afa1 Compare February 3, 2025 10:19
@enirolf enirolf requested review from jblomer and hahnjo February 4, 2025 08:18
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/src/RNTupleProcessor.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Outdated Show resolved Hide resolved
tree/ntuple/v7/inc/ROOT/RNTupleProcessor.hxx Show resolved Hide resolved
Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

In anticipation of the composition of the different processors (in
particular, the join processor), entries loading should offer the
possibility for random access. This will not change anything about the
way users can interact with the processor, which is exclusively through
the (linear) iterator.
With this change, the `RNTupleChainProcessor` iterates over other
`RNTupleProcessor` objects instead of individual ntuples. This allows us
to chain, for example, ntuples that have previously been joined using
the `RNTupleJoinProcessor`.
When no model is provided, it needs to be inferred from the page source,
which first has to be created. This model inference previously happened
at the `RNTupleProcessor::Create` factory method. Deferring this to the
`RNTupleSingleProcessor` constructor instead removes the need to create
the page source twice. In addition, when a model is provided, the page
source only gets created when the processor is actually connected.
It is currently not used, and can be re-added when we actually start
using it.
@enirolf enirolf force-pushed the ntuple-processor-chain-composition branch from e70afa1 to 3da39b3 Compare February 7, 2025 14:49
@enirolf enirolf requested a review from hahnjo February 7, 2025 14:51
@enirolf enirolf merged commit 3ebf165 into root-project:master Feb 9, 2025
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants