[ntuple] Distinguish primary and auxiliary ntuples in RNTupleProcessor::CreateJoin
#17881
+102
−106
Conversation
This file contains 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
enirolf
force-pushed
the
ntuple-processor-join-factory
branch
from
aa7e674 to
113e0e4
Compare
pcanal
reviewed
Mar 4, 2025
pcanal
reviewed
Mar 4, 2025
Test Results 18 files 18 suites 4d 11h 49m 45s ⏱️ For more details on these failures, see this check. Results for commit 0ce1f45. ♻️ This comment has been updated with latest results. |
... in the `RNTupleProcessor::CreateJoin` factory. This should make the roles of the different ntuples more clear.
enirolf
force-pushed
the
ntuple-processor-join-factory
branch
from
113e0e4 to
0ce1f45
Compare
This, way the `RNTupleJoinConstructor` itself creates a complete processor, rather than having the `RNTupleProcessor::CreateJoin` factory be responsible for this.
pcanal
reviewed
Mar 5, 2025
vepadulano
requested changes
Mar 5, 2025
| /// \param[in] primaryNTuple The name and location of the primary RNTuple, which will drive the processor iteration | ||
| /// loop. | ||
| /// \param[in] auxNTuples The names and locations of the RNTuples to join the primary RNTuple with. Which entries of | ||
| /// these auxiliary RNTuples are read is determined by the primary RNTuple, which does not necessarily happens in |
There was a problem hiding this comment.
There is a small typo, but I actually wanted to ask a clarification regarding this sentence. What does not necessarily happen in sequential order?
Suggested change
| /// these auxiliary RNTuples are read is determined by the primary RNTuple, which does not necessarily happens in | |
| /// these auxiliary RNTuples are read is determined by the primary RNTuple, which does not necessarily happen in |
| /// \param[in] auxNTUples The source specifications (name and storage location) of the auxiliary RNTuples. | ||
| /// \param[in] joinFields The names of the fields on which to join, in case the specified RNTuples are unaligned. | ||
| /// The join is made based on the combined join field values, and therefore each field has to be present in each | ||
| /// specified RNTuple. If an empty list is provided, it is assumed that the specified ntuple are fully aligned. |
There was a problem hiding this comment.
Suggested change
| /// specified RNTuple. If an empty list is provided, it is assumed that the specified ntuple are fully aligned. | |
| /// specified RNTuple. If an empty list is provided, it is assumed that the specified ntuples are fully aligned. |
| RNTupleJoinProcessor(const RNTupleOpenSpec &mainNTuple, std::string_view processorName, | ||
| std::unique_ptr<RNTupleModel> model = nullptr); | ||
| /// \param[in] models The models that specify which fields should be read by the processor, ordered according to | ||
| /// {mainNTuple, auxNTuple[0], ...} The pointer returned by RNTupleModel::MakeField can be used to access a field's |
There was a problem hiding this comment.
Suggested change
| /// {mainNTuple, auxNTuple[0], ...} The pointer returned by RNTupleModel::MakeField can be used to access a field's | |
| /// {mainNTuple, auxNTuple[0], ...}. The pointer returned by RNTupleModel::MakeField can be used to access a field's |
Comment on lines
+533
to
+534
| /// \param[in] models The models that specify which fields should be read by the processor, ordered according to | ||
| /// {mainNTuple, auxNTuple[0], ...} The pointer returned by RNTupleModel::MakeField can be used to access a field's |
There was a problem hiding this comment.
This description of the models parameter is slightly different from other descriptions of the same parameter in other places. Would it be better to homogenise them perhaps?
| @@ -19,13 +19,13 @@ | |||
|
|
|||
| namespace { | |||
| using ROOT::Experimental::RNTupleOpenSpec; | |||
| void EnsureUniqueNTupleNames(const std::vector<RNTupleOpenSpec> &ntuples) | |||
| void EnsureUniqueNTupleNames(const RNTupleOpenSpec primaryNTuple, const std::vector<RNTupleOpenSpec> &auxNTuples) | |||
There was a problem hiding this comment.
Suggested change
| void EnsureUniqueNTupleNames(const RNTupleOpenSpec primaryNTuple, const std::vector<RNTupleOpenSpec> &auxNTuples) | |
| void EnsureUniqueNTupleNames(const RNTupleOpenSpec &primaryNTuple, const std::vector<RNTupleOpenSpec> &auxNTuples) |
Comment on lines
+169
to
+172
| innerProcs.push_back( | ||
| RNTupleProcessor::CreateJoin({fNTupleNames[0], fFileNames[0]}, {{fNTupleNames[1], fFileNames[1]}}, {})); | ||
| innerProcs.push_back( | ||
| RNTupleProcessor::CreateJoin({fNTupleNames[0], fFileNames[0]}, {{fNTupleNames[1], fFileNames[1]}}, {"i"})); |
There was a problem hiding this comment.
Is this practically chaining together a join composition made of two files that are aligned with another join composition with unaligned events?
|
|
||
| auto proc = RNTupleProcessor::CreateJoin(ntuples, {}); | ||
| // Primary ntuple only | ||
| auto proc = RNTupleProcessor::CreateJoin({fNTupleNames[0], fFileNames[0]}, {}, {}); |
There was a problem hiding this comment.
I know this test was already here before, but looking at it again made me wonder. Is this just a sanity check or do we expect this syntax to be used? On the surface it looks weird that a user calls CreateJoin if there is only one RNTuple.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Grouping them all in one collection doesn't really make sense from a semantic point of view. This separation should make the roles of the different ntuples more clear.
A next consideration is the representation of the model(s). Ideally (IMO) there should just be one model passed to this factory that already represents the schema after the join. However, this would put a lot of burden on the user to create a "correct" model, so this requires a bit more thought and will be followed up on later.