From bde31d127ffbf2a3711282d0960c3937a771c78d Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Thu, 6 Feb 2025 19:13:58 +0100 Subject: [PATCH 1/4] [TTree] when merging, ignore trees without branches [TTree] when merging, do not clone branch structure if no entries Fixes #14558 Fixes #12510 [test] add fully qualified tree [tree] skip trees with zero branches in static merge By pcanal [test] amend tests with cases [tree] restore prev behavior [tree] skip "this" when Merge starts with empty-branch tree as implemented by pcanal [skip-ci] clarify comments [tree] early exit if all empty [test] fix merge substitution --- io/io/test/TFileMergerTests.cxx | 62 +++++++++++++++++++++- tree/tree/src/TTree.cxx | 92 ++++++++++++++++++++++++++++----- 2 files changed, 138 insertions(+), 16 deletions(-) diff --git a/io/io/test/TFileMergerTests.cxx b/io/io/test/TFileMergerTests.cxx index 36ecee8b76a41..6de623a60b752 100644 --- a/io/io/test/TFileMergerTests.cxx +++ b/io/io/test/TFileMergerTests.cxx @@ -1,6 +1,7 @@ #include "ROOT/TestSupport.hxx" #include "TFileMerger.h" +#include "TFileMergeInfo.h" #include "TMemFile.h" #include "TTree.h" @@ -84,12 +85,12 @@ TEST(TFileMerger, MergeSingleOnlyListed) hist3->Fill(1); hist3->Fill(1); hist3->Fill(1); hist4->Fill(1); hist4->Fill(1); hist4->Fill(1); hist4->Fill(1); a.Write(); - + TFileMerger merger; auto output = std::unique_ptr(new TFile("SingleOnlyListed.root", "RECREATE")); bool success = merger.OutputFile(std::move(output)); ASSERT_TRUE(success); - + merger.AddObjectNames("hist1"); merger.AddObjectNames("hist2"); merger.AddFile(&a, false); @@ -101,3 +102,60 @@ TEST(TFileMerger, MergeSingleOnlyListed) ASSERT_TRUE(output.get() && output->GetListOfKeys()); EXPECT_EQ(output->GetListOfKeys()->GetSize(), 2); } + +// https://github.com/root-project/root/issues/14558 aka https://its.cern.ch/jira/browse/ROOT-4716 +TEST(TFileMerger, MergeBranches) +{ + TTree atree("atree", "atitle"); + int value; + atree.Branch("a", &value); + + TTree abtree("abtree", "abtitle"); + abtree.Branch("a", &value); + abtree.Branch("b", &value); + value = 11; + abtree.Fill(); + value = 42; + abtree.Fill(); + + TTree dummy("emptytree", "emptytitle"); + TList treelist; + + // Case 1 - Static: NoBranch + NoEntries + 2 entries + treelist.Add(&dummy); + treelist.Add(&atree); + treelist.Add(&abtree); + std::unique_ptr file1(TFile::Open("b_4716.root", "RECREATE")); + auto rtree = TTree::MergeTrees(&treelist); + ASSERT_TRUE(rtree->FindBranch("a") != nullptr); + EXPECT_EQ(rtree->FindBranch("a")->GetEntries(), 2); + ASSERT_TRUE(rtree->FindBranch("b") == nullptr); + file1->Write(); + + // Case 2 - This (NoBranch) + NoEntries + 2 entries + treelist.Clear(); + treelist.Add(&atree); + treelist.Add(&abtree); + std::unique_ptr file2(TFile::Open("c_4716.root", "RECREATE")); + TFileMergeInfo info2(file2.get()); + dummy.Merge(&treelist, &info2); + ASSERT_TRUE(dummy.FindBranch("a") != nullptr); + ASSERT_TRUE(dummy.FindBranch("b") == nullptr); + EXPECT_EQ(dummy.FindBranch("a")->GetEntries(), 2); + EXPECT_EQ(atree.FindBranch("a")->GetEntries(), 2); + // atree has now 2 entries instead of zero since it was used as skeleton for dummy + file2->Write(); + + // Case 3 - This (NoEntries) + 2 entries + TTree a0tree("a0tree", "a0title"); // We cannot reuse atree since it was cannibalized by dummy + a0tree.Branch("a", &value); + treelist.Clear(); + treelist.Add(&abtree); + std::unique_ptr file3(TFile::Open("d_4716.root", "RECREATE")); + TFileMergeInfo info3(file3.get()); + a0tree.Merge(&treelist, &info3); + ASSERT_TRUE(a0tree.FindBranch("a") != nullptr); + ASSERT_TRUE(a0tree.FindBranch("b") == nullptr); + EXPECT_EQ(a0tree.FindBranch("a")->GetEntries(), 2); + file3->Write(); +} diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 483c5252b7048..cbfff6f50e8b9 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -30,7 +30,7 @@ /** \class TTree \ingroup tree -A TTree represents a columnar dataset. Any C++ type can be stored in its columns. The modern +A TTree represents a columnar dataset. Any C++ type can be stored in its columns. The modern version of TTree is RNTuple: please consider using it before opting for TTree. A TTree, often called in jargon *tree*, consists of a list of independent columns or *branches*, @@ -141,19 +141,19 @@ It is strongly recommended to persistify those as objects rather than lists of l ~~~ {.cpp} auto branch = tree.Branch( branchname, STLcollection, buffsize, splitlevel); ~~~ -`STLcollection` is the address of a pointer to a container of the standard -library such as `std::vector`, `std::list`, containing pointers, fundamental types +`STLcollection` is the address of a pointer to a container of the standard +library such as `std::vector`, `std::list`, containing pointers, fundamental types or objects. If the splitlevel is a value bigger than 100 (`TTree::kSplitCollectionOfPointers`) -then the collection will be written in split mode, i.e. transparently storing +then the collection will be written in split mode, i.e. transparently storing individual data members as arrays, therewith potentially increasing compression ratio. -### Note -In case of dynamic structures changing with each entry, see e.g. +### Note +In case of dynamic structures changing with each entry, see e.g. ~~~ {.cpp} branch->SetAddress(void *address) ~~~ -one must redefine the branch address before filling the branch +one must redefine the branch address before filling the branch again. This is done via the `TBranch::SetAddress` member function. \anchor addingacolumnofobjs @@ -186,8 +186,8 @@ Another available syntax is the following: - `p_object` is a pointer to an object. - If `className` is not specified, the `Branch` method uses the type of `p_object` to determine the type of the object. -- If `className` is used to specify explicitly the object type, the `className` - must be of a type related to the one pointed to by the pointer. It should be +- If `className` is used to specify explicitly the object type, the `className` + must be of a type related to the one pointed to by the pointer. It should be either a parent or derived class. Note: The pointer whose address is passed to `TTree::Branch` must not @@ -205,13 +205,13 @@ or not tree.Branch(branchname, &p_object); ~~~ In either case, the ownership of the object is not taken over by the `TTree`. -Even though in the first case an object is be allocated by `TTree::Branch`, +Even though in the first case an object is be allocated by `TTree::Branch`, the object will not be deleted when the `TTree` is deleted. \anchor addingacolumnoftclonesarray ## Add a column holding TClonesArray instances -*The usage of `TClonesArray` should be abandoned in favour of `std::vector`, +*The usage of `TClonesArray` should be abandoned in favour of `std::vector`, for which `TTree` has been heavily optimised, as well as `RNTuple`.* ~~~ {.cpp} @@ -347,7 +347,7 @@ int main() ~~~ ## TTree Diagram -The following diagram shows the organisation of the federation of classes related to TTree. +The following diagram shows the organisation of the federation of classes related to TTree. Begin_Macro ../../../tutorials/legacy/tree/tree.C @@ -6841,6 +6841,8 @@ bool TTree::MemoryFull(Int_t nbytes) /// /// Trees in the list can be memory or disk-resident trees. /// The new tree is created in the current directory (memory if gROOT). +/// Trees with no branches will be skipped, the branch structure +/// will be taken from the first non-zero-branch Tree of {li} TTree* TTree::MergeTrees(TList* li, Option_t* options) { @@ -6852,8 +6854,11 @@ TTree* TTree::MergeTrees(TList* li, Option_t* options) while ((obj=next())) { if (!obj->InheritsFrom(TTree::Class())) continue; TTree *tree = (TTree*)obj; + if (tree->GetListOfBranches()->IsEmpty()) + continue; // Completely ignore the empty trees. Long64_t nentries = tree->GetEntries(); - if (nentries == 0) continue; + if (newtree && nentries == 0) + continue; // If we already have the structure and we have no entry, save time and skip if (!newtree) { newtree = (TTree*)tree->CloneTree(-1, options); if (!newtree) continue; @@ -6867,7 +6872,8 @@ TTree* TTree::MergeTrees(TList* li, Option_t* options) newtree->ResetBranchAddresses(); continue; } - + if (nentries == 0) + continue; newtree->CopyEntries(tree, -1, options, true); } if (newtree && newtree->GetTreeIndex()) { @@ -6880,9 +6886,39 @@ TTree* TTree::MergeTrees(TList* li, Option_t* options) /// Merge the trees in the TList into this tree. /// /// Returns the total number of entries in the merged tree. +/// Trees with no branches will be skipped, the branch structure +/// will be taken from the first non-zero-branch Tree of {this+li} Long64_t TTree::Merge(TCollection* li, Option_t *options) { + if (fBranches.IsEmpty()) { + if (!li || li->IsEmpty()) + return 0; // Nothing to do .... + // Let's find the first non-empty + TIter next(li); + TTree *tree; + while ((tree = (TTree *)next())) { + if (tree == this || tree->GetListOfBranches()->IsEmpty()) + continue; + // We could come from a list made up of different names, the first one still wins + tree->SetName(this->GetName()); + auto prevEntries = tree->GetEntries(); + auto result = tree->Merge(li, options); + if (result != prevEntries) { + // If there is no additional entries, the first write was enough. + tree->Write(); + } + // Make sure things are really written out to disk before attempting any reading. + if (tree->GetCurrentFile()) { + tree->GetCurrentFile()->Flush(); + // Read back the complete info in this TTree, so that caller does not + // inadvertently write the empty tree. + tree->GetDirectory()->ReadTObject(this, this->GetName()); + } + return result; + } + return 0; // All trees have empty branches + } if (!li) return 0; Long64_t storeAutoSave = fAutoSave; // Disable the autosave as the TFileMerge keeps a list of key and deleting the underlying @@ -6915,11 +6951,39 @@ Long64_t TTree::Merge(TCollection* li, Option_t *options) /// info->fOutputDirectory and then overlay the new TTree information onto /// this TTree object (so that this TTree object is now the appropriate to /// use for further merging). +/// Trees with no branches will be skipped, the branch structure +/// will be taken from the first non-zero-branch Tree of {this+li} /// /// Returns the total number of entries in the merged tree. Long64_t TTree::Merge(TCollection* li, TFileMergeInfo *info) { + if (fBranches.IsEmpty()) { + if (!li || li->IsEmpty()) + return 0; // Nothing to do .... + // Let's find the first non-empty + TIter next(li); + TTree *tree; + while ((tree = (TTree *)next())) { + if (tree == this || tree->GetListOfBranches()->IsEmpty()) + continue; + // We could come from a list made up of different names, the first one still wins + tree->SetName(this->GetName()); + auto prevEntries = tree->GetEntries(); + auto result = tree->Merge(li, info); + if (result != prevEntries) { + // If there is no additional entries, the first write was enough. + tree->Write(); + } + // Make sure things are really written out to disk before attempting any reading. + info->fOutputDirectory->GetFile()->Flush(); + // Read back the complete info in this TTree, so that TFileMerge does not + // inadvertently write the empty tree. + info->fOutputDirectory->ReadTObject(this, this->GetName()); + return result; + } + return 0; // All trees have empty branches + } const char *options = info ? info->fOptions.Data() : ""; if (info && info->fIsFirst && info->fOutputDirectory && info->fOutputDirectory->GetFile() != GetCurrentFile()) { if (GetCurrentFile() == nullptr) { From 65f5533fdce16b81cc92b13669a784a61d674559 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 28 Feb 2025 16:58:47 +0100 Subject: [PATCH 2/4] [tree] emit debug warning if tree with zero branches --- tree/tree/src/TTree.cxx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index cbfff6f50e8b9..99c125a7309f7 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -6854,8 +6854,12 @@ TTree* TTree::MergeTrees(TList* li, Option_t* options) while ((obj=next())) { if (!obj->InheritsFrom(TTree::Class())) continue; TTree *tree = (TTree*)obj; - if (tree->GetListOfBranches()->IsEmpty()) + if (tree->GetListOfBranches()->IsEmpty()) { + if (gDebug > 2) { + Warning("MergeTrees","TTree %s has no branches, skipping.", tree->GetName()); + } continue; // Completely ignore the empty trees. + } Long64_t nentries = tree->GetEntries(); if (newtree && nentries == 0) continue; // If we already have the structure and we have no entry, save time and skip @@ -6898,8 +6902,12 @@ Long64_t TTree::Merge(TCollection* li, Option_t *options) TIter next(li); TTree *tree; while ((tree = (TTree *)next())) { - if (tree == this || tree->GetListOfBranches()->IsEmpty()) + if (tree == this || tree->GetListOfBranches()->IsEmpty()) { + if (gDebug > 2) { + Warning("Merge","TTree %s has no branches, skipping.", tree->GetName()); + } continue; + } // We could come from a list made up of different names, the first one still wins tree->SetName(this->GetName()); auto prevEntries = tree->GetEntries(); @@ -6966,6 +6974,9 @@ Long64_t TTree::Merge(TCollection* li, TFileMergeInfo *info) TTree *tree; while ((tree = (TTree *)next())) { if (tree == this || tree->GetListOfBranches()->IsEmpty()) + if (gDebug > 2) { + Warning("Merge","TTree %s has no branches, skipping.", tree->GetName()); + } continue; // We could come from a list made up of different names, the first one still wins tree->SetName(this->GetName()); From 9e646e75fd64c7985e25b9a9dabd018c6eab5eb2 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 28 Feb 2025 17:14:32 +0100 Subject: [PATCH 3/4] [tree] fix braces --- tree/tree/src/TTree.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 99c125a7309f7..2067b050892d8 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -6973,11 +6973,12 @@ Long64_t TTree::Merge(TCollection* li, TFileMergeInfo *info) TIter next(li); TTree *tree; while ((tree = (TTree *)next())) { - if (tree == this || tree->GetListOfBranches()->IsEmpty()) + if (tree == this || tree->GetListOfBranches()->IsEmpty()) { if (gDebug > 2) { Warning("Merge","TTree %s has no branches, skipping.", tree->GetName()); } continue; + } // We could come from a list made up of different names, the first one still wins tree->SetName(this->GetName()); auto prevEntries = tree->GetEntries(); From 7f6cce8681f89a0893ee915ecd7de61cac1318b4 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Fri, 28 Feb 2025 17:26:03 +0100 Subject: [PATCH 4/4] [tree] fix static call --- tree/tree/src/TTree.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 2067b050892d8..cc4586fc7e261 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -6856,7 +6856,7 @@ TTree* TTree::MergeTrees(TList* li, Option_t* options) TTree *tree = (TTree*)obj; if (tree->GetListOfBranches()->IsEmpty()) { if (gDebug > 2) { - Warning("MergeTrees","TTree %s has no branches, skipping.", tree->GetName()); + tree->Warning("MergeTrees","TTree %s has no branches, skipping.", tree->GetName()); } continue; // Completely ignore the empty trees. }