[df] Fix global RSampleInfo::EntryRange when processing chains - #23548
ravindra-RKB wants to merge 3 commits into
Conversation
43dc221 to
a9818ae
Compare
| if (range.second == -1) { | ||
| range.second = tree->GetEntries(); // convert '-1', i.e. 'until the end', to the actual entry number | ||
| } else if (auto chain = dynamic_cast<TChain*>(r.GetTree())) { | ||
| // The reader is iterating over a TChain (e.g. from TTreeProcessorMT global clusters). | ||
| // The entry range from the reader is global, but RSampleInfo expects local indices. | ||
| Long64_t treeOffset = chain->GetTreeOffset()[chain->GetTreeNumber()]; | ||
| range.first -= treeOffset; | ||
| range.second -= treeOffset; |
There was a problem hiding this comment.
Doesn't range.first need to be updated whether .second is -1 or not?
Is there structural guarantee that range.first and .second are greater than treeOffset? If not what is the semantic and how should it be handled?
|
Hi @pcanal, If Regarding the structural guarantee: Yes, To make this logic more robust and self-documenting, I have pushed a new commit that adds a safety check ( |
| auto makeTree = [](const std::string& fname, int offset) { | ||
| TFile f(fname.c_str(), "RECREATE"); | ||
| TTree t("t", "t"); | ||
| int x; t.Branch("x", &x); | ||
| x = offset; t.Fill(); x = offset+1; t.Fill(); | ||
| f.Write(); f.Close(); | ||
| }; | ||
| makeTree("test_issue22741_main1.root", 0); | ||
| makeTree("test_issue22741_main2.root", 2); | ||
| makeTree("test_issue22741_friend1.root", 0); | ||
| makeTree("test_issue22741_friend2.root", 2); |
There was a problem hiding this comment.
This part should be rewritten to use RAII to ensure file deletion after the test.
| TFile f(fname.c_str(), "RECREATE"); | ||
| TTree t("t", "t"); |
There was a problem hiding this comment.
Use std::unique_ptr rather than allocating TFile and TTree on the stack
| TTree t("t", "t"); | ||
| int x; t.Branch("x", &x); | ||
| x = offset; t.Fill(); x = offset+1; t.Fill(); | ||
| f.Write(); f.Close(); |
There was a problem hiding this comment.
Write is enough, Close is not necessary
| std::pair<Long64_t, Long64_t> range = r.GetEntriesRange(); | ||
| R__ASSERT(range.first >= 0); | ||
| if (range.second == -1) { | ||
| // If no explicit range was set, fBeginEntry is 0. The local range is the entire tree. | ||
| range.first = 0; | ||
| range.second = tree->GetEntries(); // convert '-1', i.e. 'until the end', to the actual entry number | ||
| } else if (auto chain = dynamic_cast<TChain*>(r.GetTree())) { | ||
| // The reader is iterating over a TChain (e.g. from TTreeProcessorMT global clusters). | ||
| // The entry range from the reader is global, but RSampleInfo expects local indices. | ||
| // TTreeProcessorMT guarantees that clusters do not cross tree boundaries. | ||
| Long64_t treeOffset = chain->GetTreeOffset()[chain->GetTreeNumber()]; | ||
| if (range.first >= treeOffset) { | ||
| range.first -= treeOffset; | ||
| range.second -= treeOffset; | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic should somehow be moved to RTTreeDS.cxx so we don't pollute RLoopManager.cxx with more TTree-specific logic (an effort that I've started some time ago, but not completed yet).
c6a5999 to
f5f1fdc
Compare
This branch has not been deployed
3 participants
Fixes #22741
When iterating over a
TChainusing global clusters (e.g., when friend trees are present andImplicitMTis enabled viaRDatasetSpec),TTreeReader::GetEntriesRange()returns a global entry range instead of local ones. However,RSampleInfo::EntryRange()expects the range to be local to the current sample (file).This PR adjusts the extracted range using
TChain::GetTreeOffsetso it is always converted back to local indices for the current sample. A unit test has been added todataframe_datasetspec.cxxto verify this behavior explicitly.