diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index 727bc530701a0..3938a799a9ad8 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, // and we are in incremental mode (because it will be reused // and has not been written to disk (for performance reason). // coverity[var_deref_model] the IsA()->InheritsFrom guarantees that the dynamic_cast will succeed. - if (ownobj && (!(type & kIncremental) || dirobj->GetFile() != target)) { + if (ownobj && (!(type & kIncremental) || dirobj->GetFile() != target->GetFile())) { dirobj->ResetBit(kMustCleanup); delete dirobj; } @@ -1169,4 +1169,3 @@ void TFileMerger::SetMsgPrefix(const char *prefix) { fMsgPrefix = prefix; } - diff --git a/io/io/test/TFileMergerTests.cxx b/io/io/test/TFileMergerTests.cxx index 137bf1f89bc34..5c5f079b73793 100644 --- a/io/io/test/TFileMergerTests.cxx +++ b/io/io/test/TFileMergerTests.cxx @@ -478,3 +478,101 @@ TEST(TFileMerger, TypeMismatchErrorTObjectWithNonTObject) fm.PartialMerge(); } } + +// Regression test for https://github.com/root-project/root/issues/23666 +TEST(TFileMerger, NestedDirMaxOpenedFiles) +{ + struct CleanupRAII { + std::vector fFiles; + ~CleanupRAII() + { + for (const auto &file : fFiles) { + gSystem->Unlink(file.c_str()); + } + } + } cleanup; + + const std::string file1 = "test_nested_merge_in1.root"; + const std::string file2 = "test_nested_merge_in2.root"; + const std::string file3 = "test_nested_merge_in3.root"; + const std::string fileOut = "test_nested_merge_out.root"; + + cleanup.fFiles.push_back(file1); + cleanup.fFiles.push_back(file2); + cleanup.fFiles.push_back(file3); + cleanup.fFiles.push_back(fileOut); + + const std::vector> inputs = {{file1, 5}, {file2, 7}, {file3, 11}}; + + // Create 3 input files with objects inside a nested directory (depth >= 2) + for (const auto &input : inputs) { + auto f = std::make_unique(input.first.c_str(), "RECREATE"); + ASSERT_FALSE(f->IsZombie()); + auto topDir = f->mkdir("nested"); + ASSERT_NE(topDir, nullptr); + auto subDir = topDir->mkdir("dir"); + ASSERT_NE(subDir, nullptr); + subDir->cd(); + + auto tree = std::make_unique("tree", "Test Tree"); + tree->SetDirectory(subDir); + + int val = 0; + tree->Branch("val", &val, "val/I"); + for (int i = 0; i < input.second; ++i) { + val = i; + tree->Fill(); + } + + auto hist = std::make_unique("hist", "Test Hist", 10, 0, 10); + hist->SetDirectory(subDir); + + for (int i = 0; i < input.second; ++i) { + hist->Fill(i % 10); + } + + f->Write(); + } + + // Merge the input files into the output file. The original issue was triggered by the use of the `-n` parameter in + // `hadd`, which opens at most `N` files at once, set to the same number of input files (3). The merger used to only + // merge data from the first two files leaving out data from the third. + { + TFileMerger merger{false}; + EXPECT_TRUE(merger.OutputFile(fileOut.c_str())); + merger.SetMaxOpenedFiles(3); + merger.AddFile(file1.c_str(), /*cpProgress*/ false); + merger.AddFile(file2.c_str(), /*cpProgress*/ false); + merger.AddFile(file3.c_str(), /*cpProgress*/ false); + EXPECT_TRUE(merger.Merge()); + } + + // Verify correct values for merged objects in the output file + { + auto fOut = std::make_unique(fileOut.c_str(), "READ"); + ASSERT_FALSE(fOut->IsZombie()); + + auto topDir = fOut->Get("nested"); + ASSERT_NE(topDir, nullptr); + auto subDir = topDir->Get("dir"); + ASSERT_NE(subDir, nullptr); + + auto tree = subDir->Get("tree"); + ASSERT_NE(tree, nullptr); + EXPECT_EQ(tree->GetEntries(), 5 + 7 + 11); + int val = 0; + tree->SetBranchAddress("val", &val); + int entryCursor{}; + for (const auto &input : inputs) { + for (int i = 0; i < input.second; ++i) { + tree->GetEntry(entryCursor); + EXPECT_EQ(val, i); + entryCursor++; + } + } + + auto hist = subDir->Get("hist"); + ASSERT_NE(hist, nullptr); + EXPECT_EQ(hist->GetEntries(), 5 + 7 + 11); + } +} diff --git a/roottest/root/io/hadd/CMakeLists.txt b/roottest/root/io/hadd/CMakeLists.txt index af76895b12106..cef429ae6d826 100644 --- a/roottest/root/io/hadd/CMakeLists.txt +++ b/roottest/root/io/hadd/CMakeLists.txt @@ -284,3 +284,28 @@ ROOTTEST_ADD_TEST(test_hadd_merge_rntuple_634, COMMAND ${ROOT_hadd_CMD} -f test_hadd_merge_rntuple_634_merged.root test_hadd_merge_rntuple_634_1.root test_hadd_merge_rntuple_634_2.root POSTCMD ${ROOT_root_CMD} -q check_merge_rntuple_634.C PASSRC 0) + + +# Regression test for https://github.com/root-project/root/issues/23666. +# "test_hadd_nested_objects_23666.root" contains a TTree in a nested directory +# "A/B/tree" with one branch "x", and one entry where "x" is 42. +configure_file(test_hadd_nested_objects_23666.root . COPYONLY) +configure_file(hadd_check_nested_objects_argn_23666.C . COPYONLY) +ROOTTEST_ADD_TEST(test_hadd_nested_objects_argn_23666, + PRECMD ${CMAKE_COMMAND} -E rm -f test_hadd_nested_objects_argn_23666_merged.root + COMMAND ${ROOT_hadd_CMD} -f -n 3 test_hadd_nested_objects_argn_23666_merged.root test_hadd_nested_objects_23666.root test_hadd_nested_objects_23666.root test_hadd_nested_objects_23666.root + POSTCMD ${ROOT_root_CMD} -q hadd_check_nested_objects_argn_23666.C + PASSRC 0) + +configure_file(hadd_check_nested_objects_128files_23666.C . COPYONLY) +configure_file(test_hadd_nested_objects_128files_23666.txt . COPYONLY) +ROOTTEST_ADD_TEST(test_hadd_nested_objects_128files_23666, + PRECMD ${CMAKE_COMMAND} -E rm -f test_hadd_nested_objects_128files_23666_merged.root + COMMAND ${ROOT_hadd_CMD} -f test_hadd_nested_objects_128files_23666_merged.root @test_hadd_nested_objects_128files_23666.txt + POSTCMD ${ROOT_root_CMD} -q hadd_check_nested_objects_128files_23666.C + PASSRC 0) +ROOTTEST_ADD_TEST(test_hadd_nested_objects_128files_n16_23666, + PRECMD ${CMAKE_COMMAND} -E rm -f test_hadd_nested_objects_128files_23666_merged.root + COMMAND ${ROOT_hadd_CMD} -f -n 16 test_hadd_nested_objects_128files_23666_merged.root @test_hadd_nested_objects_128files_23666.txt + POSTCMD ${ROOT_root_CMD} -q hadd_check_nested_objects_128files_23666.C + PASSRC 0) diff --git a/roottest/root/io/hadd/hadd_check_nested_objects_128files_23666.C b/roottest/root/io/hadd/hadd_check_nested_objects_128files_23666.C new file mode 100644 index 0000000000000..a3a04ef4d46b8 --- /dev/null +++ b/roottest/root/io/hadd/hadd_check_nested_objects_128files_23666.C @@ -0,0 +1,12 @@ +int hadd_check_nested_objects_argn_23666() +{ + auto file = std::unique_ptr(TFile::Open("test_hadd_nested_objects_128files_23666_merged.root")); + auto tree = std::unique_ptr(file->Get("A/B/tree")); + + if (auto nentries = tree->GetEntries(); nentries != 128) { + std::cerr << "Number of entries is " << nentries << " (expected 128) \n"; + return 1; + } + + return 0; +} diff --git a/roottest/root/io/hadd/hadd_check_nested_objects_argn_23666.C b/roottest/root/io/hadd/hadd_check_nested_objects_argn_23666.C new file mode 100644 index 0000000000000..10f0c49e2904f --- /dev/null +++ b/roottest/root/io/hadd/hadd_check_nested_objects_argn_23666.C @@ -0,0 +1,12 @@ +int hadd_check_nested_objects_argn_23666() +{ + auto file = std::unique_ptr(TFile::Open("test_hadd_nested_objects_argn_23666_merged.root")); + auto tree = std::unique_ptr(file->Get("A/B/tree")); + + if (auto nentries = tree->GetEntries(); nentries != 3) { + std::cerr << "Number of entries is " << nentries << " (expected 3) \n"; + return 1; + } + + return 0; +} diff --git a/roottest/root/io/hadd/test_hadd_nested_objects_128files_23666.txt b/roottest/root/io/hadd/test_hadd_nested_objects_128files_23666.txt new file mode 100644 index 0000000000000..2e06063af6390 --- /dev/null +++ b/roottest/root/io/hadd/test_hadd_nested_objects_128files_23666.txt @@ -0,0 +1,128 @@ +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root +test_hadd_nested_objects_23666.root diff --git a/roottest/root/io/hadd/test_hadd_nested_objects_23666.root b/roottest/root/io/hadd/test_hadd_nested_objects_23666.root new file mode 100644 index 0000000000000..3cd791859889e Binary files /dev/null and b/roottest/root/io/hadd/test_hadd_nested_objects_23666.root differ