From 84f552000ff62b2b57ab8d8738261772b7d6561a Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Mon, 22 Jun 2026 10:30:22 +0200 Subject: [PATCH 1/3] [tree] Add tests for GetMinimum,GetMaximum with friends This commit introduces testing for GetMinimum,GetMaximum in case the requested column belongs to a friend tree. Two tests are taken directly from the reproducer reported by a user at https://root-forum.cern.ch/t/ttree-getminimum-getmaximum-only-scan-one-file-of-a-friend-tchain/64905 A third test exercises in particular the correct updating of the branch addresses of the friend TChain when it switches to another file even though the main TChain is still traversing the same file. --- tree/tree/test/TTreeRegressions.cxx | 357 ++++++++++++++++++++++++++++ 1 file changed, 357 insertions(+) diff --git a/tree/tree/test/TTreeRegressions.cxx b/tree/tree/test/TTreeRegressions.cxx index 5aba6fe4cddf8..77e52aae230af 100644 --- a/tree/tree/test/TTreeRegressions.cxx +++ b/tree/tree/test/TTreeRegressions.cxx @@ -1,5 +1,6 @@ #include "TMemFile.h" #include "TLeaf.h" +#include "TChain.h" #include "TTree.h" #include "TInterpreter.h" #include "TSystem.h" @@ -9,7 +10,9 @@ #include "gtest/gtest.h" +#include #include +#include // ROOT-10702 TEST(TTreeRegressions, CompositeTypeWithNameClash) @@ -322,3 +325,357 @@ TEST(TTreeRegressions, DrawAutoBinning) delete h; delete gROOT->FindObject("c1"); } + +// Regressions for https://github.com/root-project/root/issues/22652 +struct RegressionGH22652 : public ::testing::Test { + + constexpr static auto fMainTreeName{"main"}; + constexpr static auto fFriendTreeName{"friend"}; + constexpr static auto fMainTreeFileName{"main_global.root"}; + constexpr static auto fNFiles{3}; + constexpr static auto fNEntriesPerFile{100}; + // file 0: w in [4,5) file 1: w in [0,1) file 2: w in [9,10) + constexpr static std::array fLowerBounds{4.0, 0.0, 9.0}; + constexpr static std::array fMainChainFileNames{"main_1.root", "main_2.root", "main_3.root"}; + constexpr static std::array fFriendChainFileNames{"friend_1.root", "friend_2.root", + "friend_3.root"}; + + constexpr static auto fNShortFiles{6}; + constexpr static auto fNEntriesPerShortFile{50}; + constexpr static std::array fShortFriendChainFileNames{ + "short_friend_1.root", "short_friend_2.root", "short_friend_3.root", + "short_friend_4.root", "short_friend_5.root", "short_friend_6.root"}; + // We set the minimum value in the second file to check that the branch address is updated by TChain::GetMinimum + constexpr static std::array fShortFriendChainValues{10, 0, 30, 40, 50, 60}; + + constexpr static std::array fMainAlternatingChainFileNames{ + "main_alternating_1.root", "main_alternating_2.root", "main_alternating_3.root"}; + constexpr static std::array fFriendAlternatingChainFileNames{ + "friend_alternating_1.root", "friend_alternating_2.root", "friend_alternating_3.root"}; + + constexpr static std::array fMainChainDifferentEntryLayout{"main_different_entry_layout_1.root", + "main_different_entry_layout_2.root"}; + + constexpr static std::array fFriendChainDifferentEntryLayout{ + "friend_different_entry_layout_01.root", "friend_different_entry_layout_02.root", + "friend_different_entry_layout_03.root", "friend_different_entry_layout_04.root", + "friend_different_entry_layout_05.root", "friend_different_entry_layout_06.root", + "friend_different_entry_layout_07.root", "friend_different_entry_layout_08.root", + "friend_different_entry_layout_09.root", "friend_different_entry_layout_10.root"}; + + static void SetUpTestSuite() + { + { + // Main TTree with cumulated number of entries + auto fd = std::make_unique(fMainTreeFileName, "RECREATE"); + auto td = std::make_unique(fMainTreeName, fMainTreeName); + double x{}; + td->Branch("x", &x); + for ([[maybe_unused]] const auto &_ : fMainChainFileNames) + for (int i = 0; i < fNEntriesPerFile; ++i) + td->Fill(); + fd->Write(); + } + + for (int i = 0; i < fNFiles; i++) { + // Trees for the main chain + { + auto fd = std::make_unique(fMainChainFileNames[i], "RECREATE"); + auto td = std::make_unique(fMainTreeName, fMainTreeName); + double x{}; + td->Branch("x", &x); + for (int j = 0; j < fNEntriesPerFile; j++) { + // x in [-0.5, 0.495] + x = j * 0.01 - 0.5; + td->Fill(); + } + fd->Write(); + } + + // Trees for the friend chain + { + auto ff = std::make_unique(fFriendChainFileNames[i], "RECREATE"); + auto tf = std::make_unique(fFriendTreeName, fFriendTreeName); + double w{}; + tf->Branch("w", &w); + for (int j = 0; j < fNEntriesPerFile; j++) { + // w in [fLowerBounds[i], fLowerBounds[i] + 1) + w = fLowerBounds[i] + j * (1.0 / fNEntriesPerFile); + tf->Fill(); + } + ff->Write(); + } + } + + // A second friend chain with the total number of entries but twice as many files (half of the entries per file) + for (auto i = 0; i < fNShortFiles; i++) { + auto fd = std::make_unique(fShortFriendChainFileNames[i], "RECREATE"); + auto td = std::make_unique(fFriendTreeName, fFriendTreeName); + double w{}; + td->Branch("w", &w); + for (int j = 0; j < fNEntriesPerShortFile; j++) { + w = fShortFriendChainValues[i]; + td->Fill(); + } + fd->Write(); + } + + // Alternating branch in main and friend trees + // Main chain + { + auto f = std::make_unique(fMainAlternatingChainFileNames[0], "RECREATE"); + auto t = std::make_unique(fMainTreeName, fMainTreeName); + double w{}; + t->Branch("w", &w); + for (int j = 0; j < fNEntriesPerFile; j++) { + // w in [fLowerBounds[i], fLowerBounds[i] + 1) + w = fLowerBounds[0] + j * (1.0 / fNEntriesPerFile); + t->Fill(); + } + f->Write(); + } + { + auto f = std::make_unique(fMainAlternatingChainFileNames[1], "RECREATE"); + auto t = std::make_unique(fMainTreeName, fMainTreeName); + for (int j = 0; j < fNEntriesPerFile; j++) { + t->Fill(); + } + f->Write(); + } + { + auto f = std::make_unique(fMainAlternatingChainFileNames[2], "RECREATE"); + auto t = std::make_unique(fMainTreeName, fMainTreeName); + double w{}; + t->Branch("w", &w); + for (int j = 0; j < fNEntriesPerFile; j++) { + // w in [fLowerBounds[i], fLowerBounds[i] + 1) + w = fLowerBounds[2] + j * (1.0 / fNEntriesPerFile); + t->Fill(); + } + f->Write(); + } + + // Friend chain + { + auto f = std::make_unique(fFriendAlternatingChainFileNames[0], "RECREATE"); + auto t = std::make_unique(fFriendTreeName, fFriendTreeName); + for (int j = 0; j < fNEntriesPerFile; j++) { + t->Fill(); + } + f->Write(); + } + { + auto f = std::make_unique(fFriendAlternatingChainFileNames[1], "RECREATE"); + auto t = std::make_unique(fFriendTreeName, fFriendTreeName); + double w{}; + t->Branch("w", &w); + for (int j = 0; j < fNEntriesPerFile; j++) { + // w in [fLowerBounds[i], fLowerBounds[i] + 1) + w = fLowerBounds[1] + j * (1.0 / fNEntriesPerFile); + // Inject max value in this file to exercise the logic with GetMaximum + if (j == (fNEntriesPerFile - 1)) + w = 150; + t->Fill(); + } + f->Write(); + } + { + auto f = std::make_unique(fFriendAlternatingChainFileNames[2], "RECREATE"); + auto t = std::make_unique(fFriendTreeName, fFriendTreeName); + for (int j = 0; j < fNEntriesPerFile; j++) { + t->Fill(); + } + f->Write(); + } + + // Alternating branch to process between main and friend chain, but the + // friend chain has a different entries layout than the main one + constexpr auto nEntriesMainDifferentLayout{10}; + constexpr auto nEntriesFriendDifferentLayout{2}; + { + auto f = std::make_unique(fMainChainDifferentEntryLayout[0], "RECREATE"); + auto t = std::make_unique(fMainTreeName, fMainTreeName); + double w{}; + t->Branch("w", &w); + for (int j = 0; j < nEntriesMainDifferentLayout; j++) { + w = -1 * j; + t->Fill(); + } + f->Write(); + } + { + auto f = std::make_unique(fMainChainDifferentEntryLayout[1], "RECREATE"); + auto t = std::make_unique(fMainTreeName, fMainTreeName); + for (int j = 0; j < nEntriesMainDifferentLayout; j++) { + t->Fill(); + } + f->Write(); + } + + { + for (auto i = 0; i < 5; i++) { + auto f = std::make_unique(fFriendChainDifferentEntryLayout[i], "RECREATE"); + auto t = std::make_unique(fFriendTreeName, fFriendTreeName); + for (int j = 0; j < nEntriesFriendDifferentLayout; j++) { + t->Fill(); + } + f->Write(); + } + + for (auto i = 5; i < 10; i++) { + auto f = std::make_unique(fFriendChainDifferentEntryLayout[i], "RECREATE"); + auto t = std::make_unique(fFriendTreeName, fFriendTreeName); + double w{}; + t->Branch("w", &w); + for (int j = 0; j < nEntriesFriendDifferentLayout; j++) { + w = i * j; + t->Fill(); + } + f->Write(); + } + } + } + + static void TearDownTestSuite() + { + std::remove(fMainTreeFileName); + + for (const auto &f : fMainChainFileNames) + std::remove(f); + + for (const auto &f : fFriendChainFileNames) + std::remove(f); + + for (const auto &f : fMainAlternatingChainFileNames) + std::remove(f); + + for (const auto &f : fFriendAlternatingChainFileNames) + std::remove(f); + + for (const auto &f : fMainChainDifferentEntryLayout) + std::remove(f); + + for (const auto &f : fFriendChainDifferentEntryLayout) + std::remove(f); + } +}; + +TEST_F(RegressionGH22652, RunMainTChain) +{ + // Main is a TChain, friend is a TChain, entries are aligned + auto m = std::make_unique(fMainTreeName); + for (const auto &fn : fMainChainFileNames) + m->Add(fn); + + auto fc = std::make_unique(fFriendTreeName); + for (const auto &fn : fFriendChainFileNames) + fc->Add(fn); + + m->AddFriend(fc.get()); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), 0.0); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 9.99); +} + +TEST_F(RegressionGH22652, RunMainTTree) +{ + // Main is a TTree, friend is a TChain, entries are aligned + + auto fm = std::make_unique(fMainTreeFileName); + std::unique_ptr m{fm->Get(fMainTreeName)}; + + auto fc = std::make_unique(fFriendTreeName); + for (const auto &fn : fFriendChainFileNames) + fc->Add(fn); + + m->AddFriend(fc.get()); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), 0.0); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 9.99); +} + +TEST_F(RegressionGH22652, TChainFriendWithShorterFiles) +{ + // Main is a TChain, friend is a TChain, total number of entries is the same + // but the friend TChain has double the number of files and half the entries + // per file. This exercises in particular the correct updating of the branch + // addresses of the friend TChain when it switches to another file even though + // the main TChain is still traversing the same file. + auto m = std::make_unique(fMainTreeName); + for (const auto &fn : fMainChainFileNames) + m->Add(fn); + + auto fc = std::make_unique(fFriendTreeName); + for (const auto &fn : fShortFriendChainFileNames) + fc->Add(fn); + + m->AddFriend(fc.get()); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), 0.0); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 60.0); +} + +TEST_F(RegressionGH22652, BranchAlternatingInMainAndFriendTChain) +{ + // Main TChain and friend TChain, same number of files, same number of entries per file, aligned. + // The branch to be processed, "w", is found alternating in trees of the main and the friend one: + // main file 0 has w, friend file 1 does not have w + // main file 1 does not have w, friend file 1 has w + // main file 2 has w, friend file 2 does not have w + // GetMinimum, GetMaximum should be able to detect the presence of branch "w" + // in the second file and connect to it to get the missing values from the + // tree of the main chain + auto m = std::make_unique(fMainTreeName); + for (const auto &fn : fMainAlternatingChainFileNames) + m->Add(fn); + + auto fc = std::make_unique(fFriendTreeName); + for (const auto &fn : fFriendAlternatingChainFileNames) + fc->Add(fn); + + m->AddFriend(fc.get()); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), 0.0); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 150.0); +} + +TEST_F(RegressionGH22652, BranchMissingInSecondFileOfChain) +{ + // Main TChain with three files: + // main file 0 has w + // main file 1 does not have w + // main file 2 has w + // GetMinimum, GetMaximum should be able to compute their values skipping + // the trees where the branch is not present + + auto m = std::make_unique(fMainTreeName); + for (const auto &fn : fMainAlternatingChainFileNames) + m->Add(fn); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), 4.0); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 9.99); +} + +TEST_F(RegressionGH22652, AlternatingBranchWithDifferentEntryLayoutInMainAndFriend) +{ + + // Main TChain has two files. Each file has 10 entries. File 1 has branch "w", File 2 does not have it + // Friend TChain has 10 files. Each files has 2 entries. + // Main and friend are "aligned" such that for each file of main there are 5 corresponding files of friend + // The first five files of friend do not have branch "w", so that GetMinimum/Maximum should get the values of "w" + // from the main chain The second five files of friend have branch "w", so that GetMinimum/GetMaximum should get the + // values of "w" from the friend chain GetMinimum/Maximum should detect that while we are in the entries of the + // second file of the main chain, we may be jumping to another file in the friend chain + + auto m = std::make_unique(fMainTreeName); + for (const auto &fn : fMainChainDifferentEntryLayout) + m->Add(fn); + auto fc = std::make_unique(fFriendTreeName); + for (const auto &fn : fFriendChainDifferentEntryLayout) + fc->Add(fn); + + m->AddFriend(fc.get()); + + EXPECT_DOUBLE_EQ(m->GetMinimum("w"), -9.); + EXPECT_DOUBLE_EQ(m->GetMaximum("w"), 9.); +} From affd55f18df3a5afcd5c6e2ad23659296ceade18 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Mon, 22 Jun 2026 10:37:58 +0200 Subject: [PATCH 2/3] [tree] Separate logic of leaf retrieval in helper methods Implement a similar strategy to what was done for GetBranch in https://github.com/root-project/root/commit/30860f32630f0ffbd3e3d00e6ddaebe333b92250 . This commit introduces non-functional changes, but a better code organization. This commit also introduces a description of the search order of TTree::GetLeaf(const char* branchname, const char *leafname) in its documentation. --- tree/tree/inc/TTree.h | 9 +- tree/tree/src/TTree.cxx | 245 +++++++++++++++++++++------------------- 2 files changed, 134 insertions(+), 120 deletions(-) diff --git a/tree/tree/inc/TTree.h b/tree/tree/inc/TTree.h index 006b7b9fc7f3b..3afa33d9e2824 100644 --- a/tree/tree/inc/TTree.h +++ b/tree/tree/inc/TTree.h @@ -200,8 +200,7 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker virtual Int_t CheckBranchAddressType(TBranch* branch, TClass* ptrClass, EDataType datatype, bool ptr); virtual TBranch *BronchExec(const char* name, const char* classname, void* addobj, bool isptrptr, Int_t bufsize, Int_t splitlevel); friend TBranch *TTreeBranchImpRef(TTree *tree, const char* branchname, TClass* ptrClass, EDataType datatype, void* addobj, Int_t bufsize, Int_t splitlevel); - Int_t SetBranchAddressImp(TBranch *branch, void* addr, TBranch** ptr); - virtual TLeaf *GetLeafImpl(const char* branchname, const char* leafname); + Int_t SetBranchAddressImp(TBranch *branch, void *addr, TBranch **ptr); Long64_t GetCacheAutoSize(bool withDefault = false); char GetNewlineValue(std::istream &inputStream); @@ -218,6 +217,12 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker virtual Int_t SetBranchAddress(const char *bname, void *add, TBranch **ptr, TClass *realClass, EDataType datatype, bool isptr, bool suppressMissingBranchError); + // Group of methods to help better separating logic used in GetLeaf + TBranch *FindBranchFromSelf(const char *branchName); + TBranch *FindBranchFromFriends(const char *branchName); + TLeaf *SearchLeafInListOfLeaves(const char *branchName, const char *leafName); + TLeaf *SearchLeafInListOfFriends(const char *branchName, const char *leafName); + class TFriendLock { // Helper class to prevent infinite recursion in the // usage of TTree Friends. Implemented in TTree.cxx. diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 7d1636aaf9052..912da3248b84a 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -4904,64 +4904,45 @@ static TBranch *R__FindBranchHelper(TObjArray *list, const char *branchname) { return nullptr; } -//////////////////////////////////////////////////////////////////////////////// -/// Return the branch that correspond to the path 'branchname', which can -/// include the name of the tree or the omitted name of the parent branches. -/// In case of ambiguity, returns the first match. -/// \sa TTree::GetBranch - -TBranch* TTree::FindBranch(const char* branchname) +TBranch *TTree::FindBranchFromSelf(const char *branchName) { - // We already have been visited while recursively looking - // through the friends tree, let return - if (kFindBranch & fFriendLockStatus) { - return nullptr; - } - - if (!branchname) - return nullptr; - - TBranch* branch = nullptr; // If the first part of the name match the TTree name, look for the right part in the - // list of branches. - // This will allow the branchname to be preceded by - // the name of this tree. - if (strncmp(fName.Data(),branchname,fName.Length())==0 && branchname[fName.Length()]=='.') { - branch = R__FindBranchHelper( GetListOfBranches(), branchname + fName.Length() + 1); - if (branch) return branch; - } + // list of branches. This will allow the branchName to be preceded by the name of this tree. + if (strncmp(fName.Data(), branchName, fName.Length()) == 0 && branchName[fName.Length()] == '.') + if (auto *br = R__FindBranchHelper(GetListOfBranches(), branchName + fName.Length() + 1)) + return br; + // If we did not find it, let's try to find the full name in the list of branches. - branch = R__FindBranchHelper(GetListOfBranches(), branchname); - if (branch) return branch; + if (auto *br = R__FindBranchHelper(GetListOfBranches(), branchName)) + return br; - // If we still did not find, let's try to find it within each branch assuming it does not the branch name. - TIter next(GetListOfBranches()); - while ((branch = (TBranch*) next())) { - TBranch* nestedbranch = branch->FindBranch(branchname); - if (nestedbranch) { + // If we still did not find, let's try to find it within each branch assuming it does not contain the branch name. + for (auto *branch : ROOT::Detail::TRangeStaticCast(*GetListOfBranches())) + if (auto *nestedbranch = branch->FindBranch(branchName)) return nestedbranch; - } - } - // Search in list of friends. + return nullptr; +} + +TBranch *TTree::FindBranchFromFriends(const char *branchName) +{ if (!fFriends) { return nullptr; } + TFriendLock lock(this, kFindBranch); - TIter nextf(fFriends); - TFriendElement* fe = nullptr; - while ((fe = (TFriendElement*) nextf())) { - TTree* t = fe->GetTree(); + for (auto *frEl : ROOT::Detail::TRangeStaticCast(*fFriends)) { + TTree *t = frEl->GetTree(); if (!t) { continue; } // If the alias is present replace it with the real name. - const char *subbranch = strstr(branchname, fe->GetName()); - if (subbranch != branchname) { + const char *subbranch = strstr(branchName, frEl->GetName()); + if (subbranch != branchName) { subbranch = nullptr; } if (subbranch) { - subbranch += strlen(fe->GetName()); + subbranch += strlen(frEl->GetName()); if (*subbranch != '.') { subbranch = nullptr; } else { @@ -4972,13 +4953,38 @@ TBranch* TTree::FindBranch(const char* branchname) if (subbranch) { name << t->GetName() << "." << subbranch; } else { - name << branchname; - } - branch = t->FindBranch(name.str().c_str()); - if (branch) { - return branch; + name << branchName; } + if (auto *br = t->FindBranch(name.str().c_str())) + return br; } + + return nullptr; +} + +//////////////////////////////////////////////////////////////////////////////// +/// Return the branch that correspond to the path 'branchname', which can +/// include the name of the tree or the omitted name of the parent branches. +/// In case of ambiguity, returns the first match. +/// \sa TTree::GetBranch + +TBranch *TTree::FindBranch(const char *branchname) +{ + // We already have been visited while recursively looking + // through the friends tree, let return + if (kFindBranch & fFriendLockStatus) { + return nullptr; + } + + if (!branchname) + return nullptr; + + if (auto *br = FindBranchFromSelf(branchname)) + return br; + + if (auto *br = FindBranchFromFriends(branchname)) + return br; + return nullptr; } @@ -6218,51 +6224,30 @@ TIterator* TTree::GetIteratorOnAllLeaves(bool dir) return new TTreeFriendLeafIter(this, dir); } -//////////////////////////////////////////////////////////////////////////////// -/// Return pointer to the 1st Leaf named name in any Branch of this -/// Tree or any branch in the list of friend trees. -/// -/// The leaf name can contain the name of a friend tree with the -/// syntax: friend_dir_and_tree.full_leaf_name -/// the friend_dir_and_tree can be of the form: -/// ~~~ {.cpp} -/// TDirectoryName/TreeName -/// ~~~ - -TLeaf* TTree::GetLeafImpl(const char* branchname, const char *leafname) +TLeaf *TTree::SearchLeafInListOfLeaves(const char *branchName, const char *leafName) { - TLeaf *leaf = nullptr; - if (branchname) { - TBranch *branch = FindBranch(branchname); - if (branch) { - leaf = branch->GetLeaf(leafname); - if (leaf) { - return leaf; - } - } - } - TIter nextl(GetListOfLeaves()); - while ((leaf = (TLeaf*)nextl())) { - if (strcmp(leaf->GetFullName(), leafname) != 0 && strcmp(leaf->GetName(), leafname) != 0) - continue; // leafname does not match GetName() nor GetFullName(), this is not the right leaf - if (branchname) { - // check the branchname is also a match + for (auto *leaf : ROOT::Detail::TRangeStaticCast(*GetListOfLeaves())) { + if (strcmp(leaf->GetFullName(), leafName) != 0 && strcmp(leaf->GetName(), leafName) != 0) + continue; // leafName does not match GetName() nor GetFullName(), this is not the right leaf + if (branchName) { + // check the branchName is also a match TBranch *br = leaf->GetBranch(); // if a quick comparison with the branch full name is a match, we are done - if (!strcmp(br->GetFullName(), branchname)) + if (!strcmp(br->GetFullName(), branchName)) return leaf; - UInt_t nbch = strlen(branchname); + UInt_t nbch = strlen(branchName); const char* brname = br->GetName(); TBranch *mother = br->GetMother(); - if (strncmp(brname,branchname,nbch)) { + if (strncmp(brname, branchName, nbch)) { if (mother != br) { const char *mothername = mother->GetName(); UInt_t motherlen = strlen(mothername); - if (!strcmp(mothername, branchname)) { + if (!strcmp(mothername, branchName)) { return leaf; - } else if (nbch > motherlen && strncmp(mothername,branchname,motherlen)==0 && (mothername[motherlen-1]=='.' || branchname[motherlen]=='.')) { + } else if (nbch > motherlen && strncmp(mothername, branchName, motherlen) == 0 && + (mothername[motherlen - 1] == '.' || branchName[motherlen] == '.')) { // The left part of the requested name match the name of the mother, let's see if the right part match the name of the branch. - if (strncmp(brname,branchname+motherlen+1,nbch-motherlen-1)) { + if (strncmp(brname, branchName + motherlen + 1, nbch - motherlen - 1)) { // No it does not continue; } // else we have match so we can proceed. @@ -6284,48 +6269,61 @@ TLeaf* TTree::GetLeafImpl(const char* branchname, const char *leafname) } return leaf; } + + return nullptr; +} + +TLeaf *TTree::SearchLeafInListOfFriends(const char *branchName, const char *leafName) +{ if (!fFriends) return nullptr; - TFriendLock lock(this,kGetLeaf); - TIter next(fFriends); - TFriendElement *fe; - while ((fe = (TFriendElement*)next())) { - TTree *t = fe->GetTree(); - if (t) { - leaf = t->GetLeaf(branchname, leafname); - if (leaf) return leaf; - } - } + // The corresponding check is in GetLeaf + TFriendLock lock(this, kGetLeaf); - //second pass in the list of friends when the leaf name - //is prefixed by the tree name + for (auto *frEl : ROOT::Detail::TRangeStaticCast(*fFriends)) + if (auto *t = frEl->GetTree()) + if (auto *leaf = t->GetLeaf(branchName, leafName)) + return leaf; + + // Second pass in the list of friends when the leaf name is prefixed by the tree name TString strippedArg; - next.Reset(); - while ((fe = (TFriendElement*)next())) { - TTree *t = fe->GetTree(); + for (auto *frEl : ROOT::Detail::TRangeStaticCast(*fFriends)) { + TTree *t = frEl->GetTree(); if (!t) continue; - const char *subname = strstr(leafname,fe->GetName()); - if (subname != leafname) continue; - Int_t l = strlen(fe->GetName()); - subname += l; - if (*subname != '.') continue; - subname++; - strippedArg += subname; - leaf = t->GetLeaf(branchname,subname); - if (leaf) return leaf; + const char *subLeafName = strstr(leafName, frEl->GetName()); + if (subLeafName != leafName) + continue; + Int_t l = strlen(frEl->GetName()); + subLeafName += l; + if (*subLeafName != '.') + continue; + subLeafName++; + strippedArg += subLeafName; + if (auto *leaf = t->GetLeaf(branchName, subLeafName)) + return leaf; } + return nullptr; } //////////////////////////////////////////////////////////////////////////////// -/// Return pointer to the 1st Leaf named name in any Branch of this -/// Tree or any branch in the list of friend trees. +/// Searches in this tree and any of its friends for a leaf named \p leafname in branch \p branchname , returns first +/// match or nullptr if no match. /// -/// The leaf name can contain the name of a friend tree with the -/// syntax: friend_dir_and_tree.full_leaf_name -/// the friend_dir_and_tree can be of the form: +/// Search order: /// -/// TDirectoryName/TreeName - +/// 1. Look for a \p branchname match (via FindBranch(branchname)): +/// a. In the list of branches of this tree +/// b. Recursively in nested branches of each branch of this tree +/// c. In the friends of this tree +/// 2. Look for matching \p branchname and \p leafname in list of leaves of this tree +/// 3. Look for matching \p branchname and \p leafname in friends of this tree (eventually calling GetLeaf on each +/// friend) +/// +/// \note \p branchname can be an empty string, in which case the function will return the first leaf with matching +/// \p leafname in any branch of this tree or any of its friends following the search order above. +/// +/// \note \p leafname can contain the name of a friend tree with the syntax: `friend_dir_and_tree.full_leaf_name`. In +/// particular, `friend_dir_and_tree` can be of the form `TDirectoryName/TreeName`. TLeaf* TTree::GetLeaf(const char* branchname, const char *leafname) { if (leafname == nullptr) return nullptr; @@ -6336,16 +6334,27 @@ TLeaf* TTree::GetLeaf(const char* branchname, const char *leafname) return nullptr; } - return GetLeafImpl(branchname,leafname); + if (auto *br = FindBranch(branchname)) + if (auto leaf = br->GetLeaf(leafname)) + return leaf; + + if (auto *leaf = SearchLeafInListOfLeaves(branchname, leafname)) + return leaf; + + if (auto *leaf = SearchLeafInListOfFriends(branchname, leafname)) + return leaf; + + return nullptr; } //////////////////////////////////////////////////////////////////////////////// -/// Return pointer to first leaf named "name" in any branch of this -/// tree or its friend trees. +/// Searches in this tree and any of its friends for a leaf named \p leafname , returns first leaf matching in any +/// branch. /// -/// \param[in] name may be in the form 'branch/leaf' +/// See TTree::GetLeaf(const char* branchname, const char *leafname) for a description of the search order. +/// +/// \note \p name may be in the form `branch/leaf` /// - TLeaf* TTree::GetLeaf(const char *name) { // Return nullptr if name is invalid or if we have @@ -6356,9 +6365,9 @@ TLeaf* TTree::GetLeaf(const char *name) std::string path(name); const auto sep = path.find_last_of('/'); if (sep != std::string::npos) - return GetLeafImpl(path.substr(0, sep).c_str(), name+sep+1); + return GetLeaf(path.substr(0, sep).c_str(), name + sep + 1); - return GetLeafImpl(nullptr, name); + return GetLeaf(nullptr, name); } //////////////////////////////////////////////////////////////////////////////// From 3d268145f0fc23c12d7d6d61a701e8b2a2b01bbc Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Wed, 2 Sep 2026 11:07:57 +0200 Subject: [PATCH 3/3] [tree] Rework Get[Minimum,Maximum] to account for arbitrary dataset combinations The processing logic for the methods is factorised out into an internal helper method. TChain does not override the methods anymore, the whole implementation is in TTree. The methods take into account the following situations: * The dataset is a TTree and contains the input column * The dataset is a TChain and contains the input column, in which case the methods detect file switching and update the leaf pointer correctly. * The dataset is a TChain, contains the input column, but some files miss it, in which case the methods skip the entries from those files. * The dataset has a friend TTree which contains the input column * The dataset is a TChain and has a friend TChain which contains the input column, in which case the methods detect file switching on the friend and update the leaf pointer correctly. * The dataset is a TChain and has a friend TChain. The input column is partially available in either the main or the friend chain. This can happen for example if the main chain has some files missing the input column and the user knowingly injects the input column in the files of the friend chain. In this case, the methods detect file switching at the boundary between files of the main chain, but also detect if there are file switches in the friend chain. Notably, the entries must still be overall aligned between the main chain and the friend one. Co-authored-by: Philippe Canal --- tree/tree/inc/TChain.h | 2 - tree/tree/inc/TTree.h | 3 + tree/tree/src/TChain.cxx | 70 --------------- tree/tree/src/TTree.cxx | 187 +++++++++++++++++++++++++++++++-------- 4 files changed, 153 insertions(+), 109 deletions(-) diff --git a/tree/tree/inc/TChain.h b/tree/tree/inc/TChain.h index d90971763f836..6773a3732e746 100644 --- a/tree/tree/inc/TChain.h +++ b/tree/tree/inc/TChain.h @@ -115,8 +115,6 @@ class TChain : public TTree { TObjArray *GetListOfFiles() const {return fFiles;} TObjArray *GetListOfLeaves() override; const char *GetAlias(const char *aliasName) const override; - Double_t GetMaximum(const char *columname) override; - Double_t GetMinimum(const char *columname) override; Int_t GetNbranches() override; Long64_t GetReadEntry() const override; TList *GetStatus() const { return fStatus; } diff --git a/tree/tree/inc/TTree.h b/tree/tree/inc/TTree.h index 3afa33d9e2824..d6fce55392fc7 100644 --- a/tree/tree/inc/TTree.h +++ b/tree/tree/inc/TTree.h @@ -185,6 +185,9 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker Int_t SetBranchAddressImp(const char *bname, void *add, TBranch **ptr, TClass *realClass, EDataType datatype, bool isptr); + // Helper method to factorize the processing logic for GetMinium, GetMaximum + double ComputeExtremum(const char *columname, double errVal, bool (*cmp)(double, double)); + protected: friend TBranch *ROOT::Internal::TreeUtils::CallBranchImpRef(TTree &tree, const char *branchname, TClass *ptrClass, EDataType datatype, void *addobj, Int_t bufsize, diff --git a/tree/tree/src/TChain.cxx b/tree/tree/src/TChain.cxx index 88da9fcc421c6..134a468afba82 100644 --- a/tree/tree/src/TChain.cxx +++ b/tree/tree/src/TChain.cxx @@ -1134,76 +1134,6 @@ TObjArray* TChain::GetListOfLeaves() return nullptr; } -//////////////////////////////////////////////////////////////////////////////// -/// Return maximum of column with name columname. - -Double_t TChain::GetMaximum(const char* columname) -{ - Double_t cmax = -DBL_MAX; - TLeaf *leaf = nullptr; - TBranch *branch = nullptr; - Int_t treenumber = -1; - for (Long64_t i = 0; i < fEntries; ++i) { - Long64_t entryNumber = this->GetEntryNumber(i); - if (entryNumber < 0) - break; - Long64_t localEntryNumber = this->LoadTree(entryNumber); - if (localEntryNumber < 0) - break; - if (treenumber != this->GetTreeNumber()) { - leaf = this->GetLeaf(columname); - if (leaf) - branch = leaf->GetBranch(); - } - treenumber = this->GetTreeNumber(); - if (!branch) - continue; - branch->GetEntry(localEntryNumber); - for (Int_t j = 0; j < leaf->GetLen(); ++j) { - Double_t val = leaf->GetValue(j); - if (val > cmax) { - cmax = val; - } - } - } - return cmax; -} - -//////////////////////////////////////////////////////////////////////////////// -/// Return minimum of column with name columname. - -Double_t TChain::GetMinimum(const char* columname) -{ - Double_t cmin = DBL_MAX; - TLeaf *leaf = nullptr; - TBranch *branch = nullptr; - Int_t treenumber = -1; - for (Long64_t i = 0; i < fEntries; ++i) { - Long64_t entryNumber = this->GetEntryNumber(i); - if (entryNumber < 0) - break; - Long64_t localEntryNumber = this->LoadTree(entryNumber); - if (localEntryNumber < 0) - break; - if (treenumber != this->GetTreeNumber()) { - leaf = this->GetLeaf(columname); - if (leaf) - branch = leaf->GetBranch(); - } - treenumber = this->GetTreeNumber(); - if (!branch) - continue; - branch->GetEntry(localEntryNumber); - for (Int_t j = 0; j < leaf->GetLen(); ++j) { - Double_t val = leaf->GetValue(j); - if (val < cmin) { - cmin = val; - } - } - } - return cmin; -} - //////////////////////////////////////////////////////////////////////////////// /// Return the number of branches of the current tree. /// diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 912da3248b84a..48191b86c5b85 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -6370,36 +6370,171 @@ TLeaf* TTree::GetLeaf(const char *name) return GetLeaf(nullptr, name); } +namespace { + //////////////////////////////////////////////////////////////////////////////// -/// Return maximum of column with name columname. -/// if the Tree has an associated TEventList or TEntryList, the maximum -/// is computed for the entries in this list. +/// \brief Helper detecting *any* file transition of a tree dataset +/// +/// This is a generic helper, works if the dataset is a TTree or a TChain, and +/// transitively detects transitions in friends. +/// +/// Comparing `TChain::GetTreeNumber()` before and after a call to +/// `TChain::LoadTree` only detects that the chain itself switched to another of +/// its own sub-trees. It does *not* detect that one of the (possibly indirect) +/// friends of the chain switched to a new file: in that case the cached +/// TLeaf/TBranch pointers become dangling even though the tree number of the +/// chain is unchanged. +/// +/// `TChain::LoadTree` (both when the chain itself moves to a new tree and, via +/// `TChain::RefreshFriendAddresses`, when only a friend was updated) calls +/// `fNotify->Notify()`. Subscribing to that notification is therefore the +/// reliable way to know that anything in the friend graph moved. +/// +/// This derives directly from TNotifyLinkBase rather than using TNotifyLink +/// because the latter would require a dictionary for the instantiation. +/// +/// We could also use +/// ``` +/// struct TLeafRefresher { +/// bool fDirty = true; +/// bool Notify() { fDirty = true; return true; } +/// }; +/// ``` +/// declared in TChain.h or InternalTreeUtils.hxx and genereate a dictionary for +/// TNotifyLink. +class FileTransitionDetector final : public TNotifyLinkBase { + /// Set to true initially so that the very first iteration performs the lookup. + bool fChanged = true; + TTree &fChain; + +public: + FileTransitionDetector(TTree &chain) : fChain(chain) { PrependLink(fChain); } + + ~FileTransitionDetector() override { RemoveLink(fChain); } + FileTransitionDetector(const FileTransitionDetector &) = delete; + FileTransitionDetector &operator=(const FileTransitionDetector &) = delete; + FileTransitionDetector(FileTransitionDetector &&) = delete; + FileTransitionDetector &operator=(FileTransitionDetector &&) = delete; + + /// Must return true: returning false would make TChain::LoadTree fail with -6. + Bool_t Notify() override + { + fChanged = true; + // Propagate to the rest of the list of subscribers, as TNotifyLink does. + if (fNext) + return fNext->Notify(); + return true; + } + + /// Returns true (once) if the chain or any of its direct or indirect friends + /// switched to a new tree since the last call. + bool CheckAndReset() + { + bool changed = fChanged; + fChanged = false; + return changed; + } +}; +} // anonymous namespace -Double_t TTree::GetMaximum(const char* columname) +//////////////////////////////////////////////////////////////////////////////// +/// Computes the extremum (minimum or maximum) for the input column name +/// +/// It takes into account the following situations: +/// +/// * The dataset is a TTree and contains the input column +/// * The dataset is a TChain and contains the input column, in which case the methods detect file switching and update +/// the leaf pointer correctly. +/// * The dataset is a TChain, contains the input column, but some files miss it, in which case the methods skip the +/// entries from those files. +/// * The dataset has a friend TTree which contains the input column +/// * The dataset is a TChain and has a friend TChain which contains the input column, in which case the methods detect +/// file switching on the friend and update the leaf pointer correctly. +/// * The dataset is a TChain and has a friend TChain. The input column is partially available in either the main or the +/// friend chain. This can happen for example if the main chain has some files missing the input column and the user +/// knowingly injects the input column in the files of the friend chain. In this case, the methods detect file switching +/// at the boundary between files of the main chain, but also detect if there are file switches in the friend chain. +/// Notably, the entries must still be overall aligned between the main chain and the friend one. +double TTree::ComputeExtremum(const char *columname, double errVal, bool (*cmp)(double, double)) { - TLeaf* leaf = this->GetLeaf(columname); + // Ensure the TTree cursor is brought back to the current entry after computing the value + struct CurrentEntryRAII { + + Long64_t fCurrentEntry; + TTree &fTree; + + CurrentEntryRAII(TTree &tree) : fCurrentEntry(tree.GetReadEntry()), fTree(tree) {} + + ~CurrentEntryRAII() { fTree.LoadTree(fCurrentEntry); } + } raii{*this}; + + // Initial lookup of the leaf name, this will find it whether it's in the + // current tree or in any of its friends + TLeaf *leaf = GetLeaf(columname); if (!leaf) { return 0; } + TBranch *branch = leaf->GetBranch(); + assert(branch); // leaf without a branch is not allowed by construction // create cache if wanted if (fCacheDoAutoInit) SetCacheSizeAux(); - TBranch* branch = leaf->GetBranch(); - Double_t cmax = -DBL_MAX; + FileTransitionDetector fileTransition{*this}; + double extremum{errVal}; for (Long64_t i = 0; i < fEntries; ++i) { - Long64_t entryNumber = this->GetEntryNumber(i); + const auto entryNumber = GetEntryNumber(i); if (entryNumber < 0) break; - branch->GetEntry(entryNumber); - for (Int_t j = 0; j < leaf->GetLen(); ++j) { - Double_t val = leaf->GetValue(j); - if (val > cmax) { - cmax = val; + const auto localEntryNumber = LoadTree(entryNumber); + if (localEntryNumber < 0) + break; + + // At every entry, we check if the processing has triggered a switch to + // a new file. We detect both a switch of the current tree in the chain + // (if this tree is a TChain) as well as a switch in any of its direct + // and indirect friends (if they are also a TChain) + if (fileTransition.CheckAndReset()) { + branch = nullptr; + leaf = GetLeaf(columname); + if (leaf) { + branch = leaf->GetBranch(); + assert(branch); // leaf without a branch is not allowed by construction + } + } + + // We accept that the leaf may not be present in one or more files in case + // it was found in a chain, we just continue processing the next entry + if (!leaf) + continue; + + // If the branch belongs to a friend, the local entry number of the friend + // may differ from the one of the chain (e.g. when the friend is indexed). + // The owning TTree has already been positioned by TChain::LoadTree, so + // its read entry is the correct one to use. + auto *owningTree = branch->GetTree(); + branch->GetEntry(owningTree->GetReadEntry()); + + auto leafLen{leaf->GetLen()}; + for (decltype(leafLen) j = 0; j < leafLen; ++j) { + auto val = leaf->GetValue(j); + if (cmp(val, extremum)) { + extremum = val; } } } - return cmax; + + return extremum; +} + +//////////////////////////////////////////////////////////////////////////////// +/// Return maximum of column with name columname. +/// if the Tree has an associated TEventList or TEntryList, the maximum +/// is computed for the entries in this list. + +Double_t TTree::GetMaximum(const char *columname) +{ + return ComputeExtremum(columname, std::numeric_limits::lowest(), [](double a, double b) { return a > b; }); } //////////////////////////////////////////////////////////////////////////////// @@ -6417,29 +6552,7 @@ Long64_t TTree::GetMaxTreeSize() Double_t TTree::GetMinimum(const char* columname) { - TLeaf* leaf = this->GetLeaf(columname); - if (!leaf) { - return 0; - } - - // create cache if wanted - if (fCacheDoAutoInit) - SetCacheSizeAux(); - - TBranch* branch = leaf->GetBranch(); - Double_t cmin = DBL_MAX; - for (Long64_t i = 0; i < fEntries; ++i) { - Long64_t entryNumber = this->GetEntryNumber(i); - if (entryNumber < 0) break; - branch->GetEntry(entryNumber); - for (Int_t j = 0;j < leaf->GetLen(); ++j) { - Double_t val = leaf->GetValue(j); - if (val < cmin) { - cmin = val; - } - } - } - return cmin; + return ComputeExtremum(columname, std::numeric_limits::max(), [](double a, double b) { return a < b; }); } ////////////////////////////////////////////////////////////////////////////////