From 53e6a9697f23a4ae0458cd22156e7a62bcee52d4 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 9 May 2025 18:46:40 +0200 Subject: [PATCH 1/6] [io] don't miss writing a histogram that is only in a last file with option -n 2 Fixes https://github.com/root-project/root/issues/9022 --- io/io/src/TFileMerger.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index 0f4ec824f14b5..6c2920136073d 100644 --- a/io/io/src/TFileMerger.cxx +++ b/io/io/src/TFileMerger.cxx @@ -542,7 +542,8 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type, keyname, keytitle); return kTRUE; } - Bool_t canBeFound = (type & kIncremental) && (current_sourcedir->GetList()->FindObject(keyname) != nullptr); + Bool_t canBeFound = (type & kIncremental) && (current_sourcedir->GetList()->FindObject(keyname) != nullptr) && + (target->GetList()->FindObject(keyname) != nullptr); // if (cl->IsTObject()) // obj->ResetBit(kMustCleanup); From e119d5dd56eff6851e25c4a77ce426c881560c90 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 14 May 2025 10:04:36 +0200 Subject: [PATCH 2/6] [io][nfc] add a print for debugging/optimization purposes to see how often the partial result is written, as pcanal suggested --- io/io/src/TFileMerger.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index 6c2920136073d..310a257bb9422 100644 --- a/io/io/src/TFileMerger.cxx +++ b/io/io/src/TFileMerger.cxx @@ -830,7 +830,8 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type, delete ndir; } } else if (!canBeFound) { // Don't write the partial result for TTree and TH1 - + if (gDebug > 0) + Info("MergeOne", "Writing partial result of %s into target", oldkeyname.Data()); if (!canBeMerged) { TIter peeknextkey(nextkey); status = WriteCycleInOrder(oldkeyname, nextkey, peeknextkey, target) && status; From dc78a10e710561e37ac32b78dba9e2a392142332 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 9 May 2025 18:59:09 +0200 Subject: [PATCH 3/6] [test][io] add merge test for single file hist with -n 2 --- io/io/test/TFileMergerTests.cxx | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/io/io/test/TFileMergerTests.cxx b/io/io/test/TFileMergerTests.cxx index e7aecd7833738..561662edea3e7 100644 --- a/io/io/test/TFileMergerTests.cxx +++ b/io/io/test/TFileMergerTests.cxx @@ -279,3 +279,39 @@ TEST(TFileMerger, SelectiveMergeWithDirectories) for (auto name : {input1, input2, output}) gSystem->Unlink(name); } + +// https://github.com/root-project/root/issues/9022 +TEST(TFileMerger, SingleHistFile) +{ + auto filename1 = "f1_9022.root"; + auto filename2 = "f2_9022.root"; + auto outname = "file9022mergeroutput.root"; + { + TFile f1(filename1, "RECREATE"); + TH1F h("h1", "h1", 1, 0, 1); + h.Write(); + f1.Close(); + TFile f2(filename2, "RECREATE"); + TH1F h2("h2", "h2", 1, 0, 1); + h2.Write(); + f2.Close(); + } + { + TFileMerger filemerger{false, false}; + filemerger.SetMaxOpenedFiles(2); + filemerger.OutputFile(std::unique_ptr{TFile::Open(outname, "RECREATE")}); + + filemerger.AddFile(filename1); + filemerger.AddFile(filename2); + + filemerger.Merge(); + } + { + TFile file(outname, "READ"); + EXPECT_NE(file.Get("h1"), nullptr); + EXPECT_NE(file.Get("h2"), nullptr); + } + gSystem->Unlink(filename1); + gSystem->Unlink(filename2); + gSystem->Unlink(outname); +} From d10d195788c74503da1de1fba6678f1344d325c0 Mon Sep 17 00:00:00 2001 From: Stephan Hageboeck Date: Sat, 12 Jul 2025 11:04:09 +0200 Subject: [PATCH 4/6] [io] Add a test for the mergeSelective tutorial. [test] fix compilation failure on alma8 wrt c++fs [test] fix failure on windows x86 just c_str() returns const std::filesystem::path::value_type * [test] fix failures on Windows [test] simplify by copying in all platforms Co-authored-by: Stephan Hageboeck --- io/io/test/CMakeLists.txt | 3 ++ io/io/test/TFileMergerTests.cxx | 79 +++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index e61201d5c9b25..93b9b91f445a5 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -10,6 +10,9 @@ ROOT_ADD_GTEST(TBufferFile TBufferFileTests.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TBufferMerger TBufferMerger.cxx LIBRARIES RIO Imt Tree) ROOT_ADD_GTEST(TBufferJSON TBufferJSONTests.cxx LIBRARIES RIO) ROOT_ADD_GTEST(TFileMerger TFileMergerTests.cxx LIBRARIES RIO Tree Hist) +if(ROOT_NEED_STDCXXFS) + target_link_libraries(TFileMerger PUBLIC stdc++fs) +endif() ROOT_ADD_GTEST(TROMemFile TROMemFileTests.cxx LIBRARIES RIO Tree) if(uring AND NOT DEFINED ENV{ROOTTEST_IGNORE_URING}) ROOT_ADD_GTEST(RIoUring RIoUring.cxx LIBRARIES RIO) diff --git a/io/io/test/TFileMergerTests.cxx b/io/io/test/TFileMergerTests.cxx index 561662edea3e7..3d790f80f13a2 100644 --- a/io/io/test/TFileMergerTests.cxx +++ b/io/io/test/TFileMergerTests.cxx @@ -8,8 +8,12 @@ #include "TFile.h" #include "TTree.h" #include "TH1.h" +#include "TNtuple.h" +#include "TProfile.h" +#include "TROOT.h" #include "TSystem.h" +#include #include #include "gtest/gtest.h" @@ -315,3 +319,78 @@ TEST(TFileMerger, SingleHistFile) gSystem->Unlink(filename2); gSystem->Unlink(outname); } + +// The merging demonstrated in tutorials/io/mergeSelective.C +TEST(TFileMerger, MergeSelectiveTutorial) +{ + using namespace std::filesystem; + struct CleanupRAII { + std::vector items; + ~CleanupRAII() + { + for (auto const &item : items) + remove(item); + } + } cleanup; + + // Create the files to be merged + const auto baseDir = path{gROOT->GetTutorialsDir()}; + std::cout << "BaseDir: " << baseDir << std::endl; + const auto file0 = baseDir / "tomerge00.root"; + const auto file1 = baseDir / "tomerge01.root"; + try { + copy(baseDir / "hsimple.root", file0); + copy(baseDir / "hsimple.root", file1); + cleanup.items.push_back(file0); + cleanup.items.push_back(file1); + } catch (filesystem_error &e) { + std::cerr << e.what() << "\n"; + } + + //------------------------------------ + // Merge only the listed objects + //------------------------------------ + { + TFileMerger fm{false}; + fm.OutputFile("exclusive.root"); + cleanup.items.push_back("exclusive.root"); + fm.AddObjectNames("hprof ntuple"); + fm.AddFile(file0.string().c_str()); + fm.AddFile(file1.string().c_str()); + // Must add new merging flag on top of the default ones + Int_t default_mode = TFileMerger::kAll | TFileMerger::kIncremental; + Int_t mode = default_mode | TFileMerger::kOnlyListed; + fm.PartialMerge(mode); + } + { + TFile file("exclusive.root"); + EXPECT_NE(file.Get("hprof"), nullptr); + EXPECT_EQ(file.Get("hpx"), nullptr); + EXPECT_EQ(file.Get("hpxpy"), nullptr); + EXPECT_NE(file.Get("ntuple"), nullptr); + } + + //------------------------------------ + // Skip merging of the listed objects + //------------------------------------ + { + TFileMerger fm{true}; + fm.OutputFile("skipped.root"); + cleanup.items.push_back("skipped.root"); + fm.AddObjectNames("hprof folder"); + fm.AddFile(file0.string().c_str()); + fm.AddFile(file1.string().c_str()); + // Must add new merging flag on top of the default ones + Int_t default_mode = TFileMerger::kAll | TFileMerger::kIncremental; + auto mode = default_mode | TFileMerger::kSkipListed; + fm.PartialMerge(mode); + fm.Reset(); + } + { + TFile file("skipped.root"); + EXPECT_EQ(file.Get("hprof"), nullptr); + EXPECT_NE(file.Get("hpx"), nullptr); + EXPECT_NE(file.Get("hpxpy"), nullptr); + EXPECT_NE(file.Get("ntuple"), nullptr); + } +} From cb9dfb2403b82358e98954f24042c14135ab2a68 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Wed, 30 Jul 2025 13:39:27 -0500 Subject: [PATCH 5/6] TFileMerger: canBeFound check only target. With the current state of the code , the main purpose (left , in previous incarnation it had a wider role) for 'canBeFound' is to detect that the object is an object owned by the file (Hist and TTree) and does not need to be written to the output and for Hist and TTree the 'write' to the output is associated with making it known to the output (have its own copy of the object). As such it actually the target we need to look at and not the source. (also the type & kIncremental makes it that it was skipping the writing only for either (the first if in append mode) or (the second and subsequent file if in regular mode) --- io/io/src/TFileMerger.cxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index 310a257bb9422..ab788daf11ec2 100644 --- a/io/io/src/TFileMerger.cxx +++ b/io/io/src/TFileMerger.cxx @@ -542,8 +542,7 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type, keyname, keytitle); return kTRUE; } - Bool_t canBeFound = (type & kIncremental) && (current_sourcedir->GetList()->FindObject(keyname) != nullptr) && - (target->GetList()->FindObject(keyname) != nullptr); + Bool_t canBeFound = (type & kIncremental) && (target->GetList()->FindObject(keyname) != nullptr); // if (cl->IsTObject()) // obj->ResetBit(kMustCleanup); From 989b730fcb738c33384649786ad1c231c0fea7bd Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Thu, 31 Jul 2025 09:13:51 +0200 Subject: [PATCH 6/6] [nfc] clarify code comment previous was a remnant of old canBeFound meaning, see https://github.com/root-project/root/commit/c4d94efa7a3e44c25f21369807eb2ee59bf43f07#diff-765b2bfb5aed033baddbf7036ddda5a103b3b82fa5456b462b74b31cd79dc51b --- io/io/src/TFileMerger.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index ab788daf11ec2..bd517590e42e8 100644 --- a/io/io/src/TFileMerger.cxx +++ b/io/io/src/TFileMerger.cxx @@ -828,7 +828,7 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type, ndir->ResetBit(kMustCleanup); delete ndir; } - } else if (!canBeFound) { // Don't write the partial result for TTree and TH1 + } else if (!canBeFound) { // object (TTree, TH1) is not yet owned by the target, thus write it if (gDebug > 0) Info("MergeOne", "Writing partial result of %s into target", oldkeyname.Data()); if (!canBeMerged) {