diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 86e93e6457eb7..fa8592935c973 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -4116,13 +4116,19 @@ TFile *TFile::Open(const char *url, Option_t *options, const char *ftitle, ssize_t len = getxattr(fileurl.GetFile(), "eos.url.xroot", nullptr, 0); if (len > 0) { std::string xurl(len, 0); - if (getxattr(fileurl.GetFile(), "eos.url.xroot", &xurl[0], len) == len) { - if ((f = TFile::Open(xurl.c_str(), options, ftitle, compress, netopt))) { - if (!f->IsZombie()) { - return f; - } else { - delete f; - f = nullptr; + std::string fileNameFromUrl{fileurl.GetFile()}; + if (getxattr(fileNameFromUrl.c_str(), "eos.url.xroot", &xurl[0], len) == len) { + // Sometimes the `getxattr` call may return an invalid URL due + // to the POSIX attribute not being yet completely filled by EOS. + if (auto baseName = fileNameFromUrl.substr(fileNameFromUrl.find_last_of("/") + 1); + std::equal(baseName.crbegin(), baseName.crend(), xurl.crbegin())) { + if ((f = TFile::Open(xurl.c_str(), options, ftitle, compress, netopt))) { + if (!f->IsZombie()) { + return f; + } else { + delete f; + f = nullptr; + } } } } diff --git a/tree/dataframe/src/RLoopManager.cxx b/tree/dataframe/src/RLoopManager.cxx index f0fae786055c6..ba16065253714 100644 --- a/tree/dataframe/src/RLoopManager.cxx +++ b/tree/dataframe/src/RLoopManager.cxx @@ -42,6 +42,25 @@ #include "ROOT/RNTupleDS.hxx" #endif +#ifdef R__UNIX +// Functions needed to perform EOS XRootD redirection in ChangeSpec +#include +#include "TEnv.h" +#include "TSystem.h" +#ifndef R__FBSD +#include +#else +#include +#endif +#ifdef R__MACOSX +/* On macOS getxattr takes two extra arguments that should be set to 0 */ +#define getxattr(path, name, value, size) getxattr(path, name, value, size, 0u, 0) +#endif +#ifdef R__FBSD +#define getxattr(path, name, value, size) extattr_get_file(path, EXTATTR_NAMESPACE_USER, name, value, size) +#endif +#endif + #include #include #include @@ -403,6 +422,38 @@ RLoopManager::RLoopManager(ROOT::RDF::Experimental::RDatasetSpec &&spec) ChangeSpec(std::move(spec)); } +#ifdef R__UNIX +namespace { +std::optional GetRedirectedSampleId(std::string_view path, std::string_view datasetName) +{ + // Mimick the redirection done in TFile::Open to see if the path points to a FUSE-mounted EOS path. + // If so, we create a redirected sample ID with the full xroot URL. + TString expandedUrl(path.data()); + gSystem->ExpandPathName(expandedUrl); + if (gEnv->GetValue("TFile.CrossProtocolRedirects", 1) == 1) { + TUrl fileurl(expandedUrl, /* default is file */ kTRUE); + if (strcmp(fileurl.GetProtocol(), "file") == 0) { + ssize_t len = getxattr(fileurl.GetFile(), "eos.url.xroot", nullptr, 0); + if (len > 0) { + std::string xurl(len, 0); + std::string fileNameFromUrl{fileurl.GetFile()}; + if (getxattr(fileNameFromUrl.c_str(), "eos.url.xroot", &xurl[0], len) == len) { + // Sometimes the `getxattr` call may return an invalid URL due + // to the POSIX attribute not being yet completely filled by EOS. + if (auto baseName = fileNameFromUrl.substr(fileNameFromUrl.find_last_of("/") + 1); + std::equal(baseName.crbegin(), baseName.crend(), xurl.crbegin())) { + return xurl + '/' + datasetName.data(); + } + } + } + } + } + + return std::nullopt; +} +} // namespace +#endif + /** * @brief Changes the internal TTree held by the RLoopManager. * @@ -441,6 +492,11 @@ void RLoopManager::ChangeSpec(ROOT::RDF::Experimental::RDatasetSpec &&spec) // is exposed to users via RSampleInfo and DefinePerSample). const auto sampleId = files[i] + '/' + trees[i]; fSampleMap.insert({sampleId, &sample}); +#ifdef R__UNIX + // Also add redirected EOS xroot URL when available + if (auto redirectedSampleId = GetRedirectedSampleId(files[i], trees[i])) + fSampleMap.insert({redirectedSampleId.value(), &sample}); +#endif } } SetTree(std::move(chain)); diff --git a/tree/tree/src/TChain.cxx b/tree/tree/src/TChain.cxx index 2533490418e3e..afa8282e18764 100644 --- a/tree/tree/src/TChain.cxx +++ b/tree/tree/src/TChain.cxx @@ -985,7 +985,18 @@ Long64_t TChain::GetEntries() const return fProofChain->GetEntries(); } if (fEntries == TTree::kMaxEntries) { - const_cast(this)->LoadTree(TTree::kMaxEntries-1); + // If the following is true, we are within a recursion about friend, + // and `LoadTree` will be no-op. + if (kLoadTree & fFriendLockStatus) + return fEntries; + const auto readEntry = fReadEntry; + auto *thisChain = const_cast(this); + thisChain->LoadTree(TTree::kMaxEntries - 1); + thisChain->InvalidateCurrentTree(); + if (readEntry >= 0) + thisChain->LoadTree(readEntry); + else + thisChain->fReadEntry = readEntry; } return fEntries; } diff --git a/tree/treeplayer/src/TTreeIndex.cxx b/tree/treeplayer/src/TTreeIndex.cxx index 15d6f248012e0..db0db91c6f73c 100644 --- a/tree/treeplayer/src/TTreeIndex.cxx +++ b/tree/treeplayer/src/TTreeIndex.cxx @@ -345,7 +345,7 @@ Long64_t TTreeIndex::GetEntryNumberFriend(const TTree *parent) if (!parent) return -3; // We reached the end of the parent tree Long64_t pentry = parent->GetReadEntry(); - if (pentry >= parent->GetEntries()) + if (pentry >= parent->GetEntriesFast()) return -2; GetMajorFormulaParent(parent); GetMinorFormulaParent(parent); diff --git a/tree/treeplayer/test/CMakeLists.txt b/tree/treeplayer/test/CMakeLists.txt index 0f53d7644284b..4edf2fda38cfc 100644 --- a/tree/treeplayer/test/CMakeLists.txt +++ b/tree/treeplayer/test/CMakeLists.txt @@ -23,3 +23,5 @@ endif() ROOT_ADD_GTEST(ttreeindex_clone ttreeindex_clone.cxx LIBRARIES TreePlayer) ROOT_ADD_GTEST(ttreereader_friends ttreereader_friends.cxx LIBRARIES TreePlayer) + +ROOT_ADD_GTEST(ttreeindex_getlistoffriends ttreeindex_getlistoffriends.cxx LIBRARIES TreePlayer) diff --git a/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx b/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx new file mode 100644 index 0000000000000..4685c93409c61 --- /dev/null +++ b/tree/treeplayer/test/ttreeindex_getlistoffriends.cxx @@ -0,0 +1,114 @@ +#include "TBranch.h" +#include "TChain.h" +#include "TCollection.h" // TRangeDynCast +#include "TFile.h" +#include "TFriendElement.h" +#include "TObjArray.h" +#include "TTree.h" + +#include "gtest/gtest.h" + +#include +#include +#include + +void write_data(std::string_view treename, std::string_view filename) +{ + TFile f{filename.data(), "update"}; + TTree t{treename.data(), treename.data()}; + int runNumber{}; + int eventNumber{}; + float val{}; + t.Branch("runNumber", &runNumber, "runNumber/I"); + t.Branch("eventNumber", &eventNumber, "eventNumber/I"); + t.Branch("val", &val, "val/F"); + if (treename == "main") { + for (auto rn = 0; rn < 3; rn++) { + runNumber = rn; + for (auto en = 0; en < 5; en++) { + eventNumber = en; + val = en * rn; + t.Fill(); + } + } + } else { + for (auto rn = 0; rn < 3; rn++) { + runNumber = rn; + for (auto en = 4; en >= 0; en--) { + eventNumber = en; + val = en * rn; + t.Fill(); + } + } + } + + f.Write(); +} + +struct TTreeIndexGH_17820 : public ::testing::Test { + constexpr static auto fFileName{"ttreeindex_getlistoffriends_gh_17820.root"}; + constexpr static auto fMainName{"main"}; + constexpr static auto fFriendName{"friend"}; + + static void SetUpTestCase() + { + write_data(fMainName, fFileName); + write_data(fFriendName, fFileName); + } + + static void TearDownTestCase() { std::remove(fFileName); } +}; + +void expect_branch_names(const TObjArray *branches, const std::vector &branchNames) +{ + auto nBranchNames = branchNames.size(); + decltype(nBranchNames) nBranches{}; + for (const auto *br : TRangeDynCast(branches)) { + EXPECT_STREQ(br->GetName(), branchNames[nBranches].c_str()); + nBranches++; + } + EXPECT_EQ(nBranches, nBranchNames); +} + +// Regression test for https://github.com/root-project/root/issues/17820 +TEST_F(TTreeIndexGH_17820, RunTest) +{ + TChain mainChain{fMainName}; + mainChain.AddFile(fFileName); + + TChain friendChain{fFriendName}; + friendChain.AddFile(fFileName); + friendChain.BuildIndex("runNumber", "eventNumber"); + + mainChain.AddFriend(&friendChain); + + // Calling GetEntries used to mess with the fTree data member of the main + // chain, not connecting it to the friend chain and thus losing the list + // of friends. This in turn corrupted the list of branches. + mainChain.GetEntries(); + + const auto *listOfBranches = mainChain.GetListOfBranches(); + ASSERT_TRUE(listOfBranches); + + const std::vector expectedNames{"runNumber", "eventNumber", "val"}; + + expect_branch_names(listOfBranches, expectedNames); + + const auto *curTree = mainChain.GetTree(); + ASSERT_TRUE(curTree); + + const auto *listOfFriends = mainChain.GetTree()->GetListOfFriends(); + ASSERT_TRUE(listOfFriends); + EXPECT_EQ(listOfFriends->GetEntries(), 1); + + auto *friendTree = dynamic_cast(dynamic_cast(listOfFriends->At(0))->GetTree()); + ASSERT_TRUE(friendTree); + + EXPECT_STREQ(friendTree->GetName(), fFriendName); + const auto *friendFile = friendTree->GetCurrentFile(); + ASSERT_TRUE(friendFile); + EXPECT_STREQ(friendFile->GetName(), fFileName); + + const auto *friendBranches = friendTree->GetListOfBranches(); + expect_branch_names(friendBranches, expectedNames); +}