From f7a9832d04565037fe47ca09fd08dbdb5d65c994 Mon Sep 17 00:00:00 2001 From: Sandro Wenzel Date: Tue, 28 Jul 2026 13:03:50 +0200 Subject: [PATCH 1/2] AODBcRewriter: remap every index column an output table carries (O2-7098) Stage 1b (added in #2370) reorders O2track_iu/O2mfttrack/O2fwdtrack to keep the "-1" ambiguous group contiguous, but wrote each table as soon as it was planned. O2fwdtrack references O2mfttrack (reordered later in the same loop) and itself (fIndexFwdTracks_MatchMCHTrack); neither permutation existed yet at write time, so both columns kept pre-reorder row numbers -- in range, so AODBcRewriterValidate passed, while every global muon got a foreign MFT/MCH leg (Maurice Coquet, sameParticle=0 on an anchored pO sample; patch proposal #2418). This fires whenever Stage 1b's regroup runs, independent of any BC/collision reorder, so every merged MC AO2D since 4 Jun is affected. Fix: * kIndexRefs: one registry of "which fIndex* column points at which table", used by the rewriter, the validator, and a new drift guard (an unregistered fIndex* column now fails the test instead of silently mis-linking data). * buildRemaps() derives a table's full remap set from that registry; the old "primary index + optional extras" split in rewriteTable is gone, so a stage can no longer forget a column the way Stage 1b forgot O2fwdtrack's. * Planning and writing are separate phases: every stage publishes its row permutation before anything is written, so forward/self references resolve correctly. New AODBcRewriterCheckLinks(in, out): fingerprints each row's non-index payload and checks, per table, that (row, referenced row) pairs survive the rewrite -- the only check that can see this bug class, since the corrupted indices were structurally valid. Needs only the input file, so it also runs on real production AO2Ds. MC/utils/tests/ adds the executable test this tool never had: a synthetic AO2D covering every pathology (duplicate/non-monotonic BCs, duplicate MC collisions, split -1 groups, fwd<->MFT and fwd->fwd references, ...), driven by run_aodbcrewriter_tests.sh -- ROOT only, seconds to run, nothing binary committed. Verified against the pre-fix rewriter and a real anchored pp production (run 553185, LHC24al apass1): sameParticle collapses to 0 with the shipped code and is fully restored with this fix, in every scenario tested, including one engineered to contain genuine duplicate BCs. Diagnosis and original patch: Maurice Coquet (#2418). Co-Authored-By: Claude Opus 5 --- MC/utils/AODBcRewriter.C | 1026 ++++++++++++++------- MC/utils/CLAUDE.md | 330 +++++-- MC/utils/tests/makeTestAOD.C | 336 +++++++ MC/utils/tests/run_aodbcrewriter_tests.sh | 105 +++ MC/utils/tests/testAODBcRewriter.C | 294 ++++++ 5 files changed, 1682 insertions(+), 409 deletions(-) create mode 100644 MC/utils/tests/makeTestAOD.C create mode 100755 MC/utils/tests/run_aodbcrewriter_tests.sh create mode 100644 MC/utils/tests/testAODBcRewriter.C diff --git a/MC/utils/AODBcRewriter.C b/MC/utils/AODBcRewriter.C index 16c7451ef..f03d013a1 100644 --- a/MC/utils/AODBcRewriter.C +++ b/MC/utils/AODBcRewriter.C @@ -39,6 +39,25 @@ // Unrelated tables — tables with no dependency on BCs or MCCollisions are // copied verbatim. // +// PLAN, THEN WRITE — every stage above only decides row orders and publishes +// the resulting permutations; processDF writes all tables +// afterwards. References are not a DAG: O2fwdtrack points at +// O2mfttrack (reordered in the same stage) and at itself, so a +// stage that wrote as it went could not remap them. It did not, +// and every global muon in every merged MC AO2D between June and +// July 2026 got a foreign MFT and MCH leg (O2-7098). +// +// ONE INDEX REGISTRY — kIndexRefs lists every fIndex* column and the table it +// points at. buildRemaps() turns it into a table's remap set, the +// validator range-checks against it, and any fIndex* column NOT in +// it is reported loudly. Do not reintroduce per-stage lists of +// "indices this stage knows how to remap". +// +// TESTING — MC/utils/tests/run_aodbcrewriter_tests.sh (ROOT only, seconds). +// AODBcRewriterCheckLinks(in, out) is the check that sees a +// mis-remapped index; the output-only checks cannot, because a +// wrong row number is still a valid one. +// // ----------------------------------------------------------------------------- // DATA MODEL DEPENDENCY GRAPH (relevant subset) // ----------------------------------------------------------------------------- @@ -75,6 +94,7 @@ #include "TTree.h" #include "TGrid.h" #include +#include #include #include #include @@ -172,6 +192,89 @@ static bool isPasteJoinChild(const std::string &tname) { return false; } +// ---------------------------------------------------------------------------- +// INDEX-REFERENCE REGISTRY — the single source of truth for "which fIndex* +// column points into which table". +// +// This ONE list drives three things: +// +// * the rewriter — buildRemaps() turns it into the complete set of value +// remaps for a table, so a reference can no longer be +// forgotten. Forgetting one is the O2-7098 bug class: +// Stage 1b reordered O2fwdtrack but nobody remapped its +// fIndexMFTTracks / fIndexFwdTracks_MatchMCHTrack, so every +// global muon silently got the wrong MFT and MCH leg. The +// values stayed in range, so the range check below passed. +// * the validator — the generic in-range check. +// * the drift guard — any fIndex* branch NOT listed here is reported, so a +// schema addition breaks the test instead of quietly +// producing mis-linked data. +// +// The value is a list of candidate table-name prefixes; the first one that +// resolves in this DF wins (that is how fIndexTracks* prefers O2track_iu over +// O2track). +// +// Referent resolution (isTableNamed): a tree named K is the table named by +// prefix P when K == P or K == P + "_" (the AO2D schema-version +// suffix). That deliberately separates O2mfttrack_001 from O2mfttrackcov, +// O2bc_001 from O2bcflag, and O2mccollision_001 from O2mccollisionlabel — a +// plain BeginsWith() would confuse them. +static const std::vector>> kIndexRefs = { + { "fIndexBCs", { "O2bc" } }, + { "fIndexBC", { "O2bc" } }, + { "fIndexSliceBCs", { "O2bc" } }, + { "fIndexCollisions", { "O2collision" } }, + { "fIndexCollision", { "O2collision" } }, + { "fIndexMcCollisions", { "O2mccollision" } }, + { "fIndexMcParticles", { "O2mcparticle" } }, + { "fIndexArrayMcParticles", { "O2mcparticle" } }, + // O2mcparticle intra-table links (mother list + [first,last] daughter slice). + { "fIndexArray_Mothers", { "O2mcparticle" } }, + { "fIndexSlice_Daughters", { "O2mcparticle" } }, + { "fIndexTracks", { "O2track_iu", "O2track" } }, + { "fIndexTracks_0", { "O2track_iu", "O2track" } }, + { "fIndexTracks_1", { "O2track_iu", "O2track" } }, + { "fIndexTracks_2", { "O2track_iu", "O2track" } }, + { "fIndexTracks_Pos", { "O2track_iu", "O2track" } }, + { "fIndexTracks_Neg", { "O2track_iu", "O2track" } }, + { "fIndexTracks_Bach", { "O2track_iu", "O2track" } }, + { "fIndexTracks_ITS", { "O2track_iu", "O2track" } }, + { "fIndexMFTTracks", { "O2mfttrack" } }, + { "fIndexFwdTracks", { "O2fwdtrack" } }, + { "fIndexFwdTracks_MatchMCHTrack", { "O2fwdtrack" } }, + { "fIndexV0s", { "O2v0" } }, + { "fIndexCascades", { "O2cascade" } }, + { "fIndexDecay3Bodys", { "O2decay3body" } }, +}; + +// K names the table with prefix P? See the comment on kIndexRefs. +static bool isTableNamed(const std::string &key, const std::string &prefix) { + if (key == prefix) return true; + if (key.size() <= prefix.size() + 1) return false; + if (key.compare(0, prefix.size(), prefix) != 0) return false; + if (key[prefix.size()] != '_') return false; + for (size_t i = prefix.size() + 1; i < key.size(); ++i) + if (!std::isdigit(static_cast(key[i]))) return false; + return true; +} + +// Every fIndex* branch of t that is NOT covered by kIndexRefs. "*_size" count +// branches of VLA index arrays are not references and are excluded. A non-empty +// result means the schema grew a link the tool does not know how to follow. +static std::vector unregisteredIndexBranches(TTree *t) { + std::vector out; + if (!t) return out; + for (auto *obj : *t->GetListOfBranches()) { + std::string bname = static_cast(obj)->GetName(); + if (bname.rfind("fIndex", 0) != 0) continue; + if (bname.size() > 5 && bname.compare(bname.size() - 5, 5, "_size") == 0) continue; + bool known = false; + for (auto &kv : kIndexRefs) if (kv.first == bname) { known = true; break; } + if (!known) out.push_back(bname); + } + return out; +} + // ============================================================================ // SECTION 2 — Generic ROOT branch I/O helpers // ============================================================================ @@ -248,13 +351,6 @@ static void writeAsInt(void *buf, ScalarTag tag, Long64_t val) { } } -// Remap a single Int_t index value through a PermMap. Returns -1 for any -// out-of-range or already-invalid (negative) value. -static Int_t remapIdx(Int_t val, const PermMap &perm) { - if (val < 0 || (size_t)val >= perm.size()) return -1; - return perm[(size_t)val]; -} - // A description of one branch in a tree: its name, scalar type tag, byte // size, and whether it is a VLA (variable-length array). For VLAs we also // keep the name of the count branch and the maximum observed element count @@ -341,40 +437,59 @@ static std::vector describeBranches(TTree *tree) { // SECTION 3 — Table rewriting engine // ============================================================================ // -// rewriteTable() is the single generic function that handles any table. +// writeTable() is the single generic function that writes any table. // It takes: -// - src : the source TTree -// - dirOut : directory to write the output TTree into -// - rowOrder : which source rows to include and in what order -// (a vector of source row indices, possibly a subset) -// - indexBranch : name of the index branch to remap, or "" if none -// - parentPerm : PermMap for remapping that index (may be empty) -// - extraRemaps : additional index columns to remap in-place via their own -// PermMaps (used for intra-table and cross-table indices -// that are not the primary sort key, e.g. mother/daughter -// indices in O2mcparticle or fIndexMcParticles in labels) +// - src : the source TTree +// - dirOut : directory to write the output TTree into +// - rowOrder : which source rows to include and in what order +// (a vector of source row indices, possibly a subset) +// - remaps : the index columns to remap in-place, each with its own PermMap // -// It returns the permutation of source rows implied by rowOrder, expressed -// as a PermMap: perm[srcRow] = outputRow, -1 if the row was dropped. - -// Describes one extra index column to remap independently of the sort key. -struct ExtraRemap { - std::string branchName; // branch whose integer values to remap - const PermMap *perm; // remapping table: newVal = (*perm)[oldVal] +// There is deliberately NO "primary index" special case. Every index column — +// the one the table was sorted by as much as an intra-table mother/daughter +// link — goes through the same `remaps` list, which callers obtain from +// buildRemaps() and therefore from the kIndexRefs registry. The previous +// design had one privileged index plus an optional list of "extra" ones, and +// every stage had to remember to populate the extras; Stage 1b did not, which +// is precisely how O2fwdtrack's match indices were left dangling (O2-7098). + +// Describes one index column to remap. +struct IndexRemap { + std::string branchName; // branch whose integer values to remap + const PermMap *perm; // remapping table: newVal = (*perm)[oldVal] }; -static PermMap rewriteTable(TTree *src, TDirectory *dirOut, - const std::vector &rowOrder, - const std::string &indexBranch, - const PermMap &parentPerm, - const std::vector &extraRemaps = {}) { +// The row permutation implied by a rowOrder: perm[srcRow] = outRow, -1 if the +// row is not emitted. +static PermMap permFromRowOrder(Long64_t nSrc, + const std::vector &rowOrder) { + PermMap perm(nSrc, -1); + for (Long64_t outRow = 0; outRow < (Long64_t)rowOrder.size(); ++outRow) + perm[rowOrder[outRow]] = (Int_t)outRow; + return perm; +} - Long64_t nSrc = src->GetEntries(); +// Remap n consecutive integers of the given type held in buf. Out-of-range and +// already-invalid values become -1 (the AO2D "no link" sentinel). +// Returns how many values actually changed. +static Long64_t remapBuffer(void *buf, ScalarTag tag, size_t elemSize, int n, + const PermMap &perm) { + Long64_t changed = 0; + auto *bytes = static_cast(buf); + for (int j = 0; j < n; ++j) { + void *slot = bytes + (size_t)j * elemSize; + Long64_t v = readAsInt(slot, tag); + Long64_t nv = (v < 0 || (size_t)v >= perm.size()) ? -1 : perm[(size_t)v]; + if (nv != v) { writeAsInt(slot, tag, nv); ++changed; } + } + return changed; +} - // Build the inverse permutation (srcRow → outRow) from rowOrder - PermMap srcToOut(nSrc, -1); - for (Long64_t outRow = 0; outRow < (Long64_t)rowOrder.size(); ++outRow) - srcToOut[rowOrder[outRow]] = (Int_t)outRow; +static void writeTable(TTree *src, TDirectory *dirOut, + const std::vector &rowOrder, + const std::vector &remaps = {}) { + + Long64_t nSrc = src->GetEntries(); // Describe all branches auto descs = describeBranches(src); @@ -422,24 +537,9 @@ static PermMap rewriteTable(TTree *src, TDirectory *dirOut, dirOut->cd(); TTree *out = src->CloneTree(0, "fast"); - // Find the index branch (if any) — we will update its value on the fly - ScalarTag idxTag = ScalarTag::kUnknown; - std::vector newIdxBuf; - TBranch *outIdxBr = nullptr; - if (!indexBranch.empty()) { - TBranch *inIdxBr = src->GetBranch(indexBranch.c_str()); - TLeaf *idxLeaf = inIdxBr ? static_cast(inIdxBr->GetListOfLeaves()->At(0)) : nullptr; - idxTag = idxLeaf ? tagOf(idxLeaf) : ScalarTag::kUnknown; - if (idxTag != ScalarTag::kUnknown) { - newIdxBuf.assign(byteSize(idxTag), 0); - outIdxBr = out->GetBranch(indexBranch.c_str()); - if (outIdxBr) outIdxBr->SetAddress(newIdxBuf.data()); - } - } - - // Set all other output branch addresses to the same data buffers as input + // Output branches share the input buffers, so an in-place remap below is + // what out->Fill() sees. for (auto &io : ios) { - if (io.desc.name == indexBranch) continue; // handled separately above TBranch *outBr = out->GetBranch(io.desc.name.c_str()); if (!outBr) { std::cerr << " [warn] no output branch for " << io.desc.name << "\n"; continue; } outBr->SetAddress(io.dataBuf.data()); @@ -449,46 +549,36 @@ static PermMap rewriteTable(TTree *src, TDirectory *dirOut, } } + // Resolve each remap to its BranchIO once, up front, instead of searching the + // branch list per row (O2mcparticle has millions of rows). + struct ResolvedRemap { BranchIO *io; const PermMap *perm; }; + std::vector resolved; + for (auto &r : remaps) { + BranchIO *found = nullptr; + for (auto &io : ios) if (io.desc.name == r.branchName) { found = &io; break; } + if (!found) continue; // branch absent (caller probed, so this is unusual) + // Index columns are signed 32-bit in the AO2D schema. Refuse to touch + // anything else rather than write -1 into an unsigned field. + if (found->desc.tag != ScalarTag::kInt && found->desc.tag != ScalarTag::kShort && + found->desc.tag != ScalarTag::kLong64) { + std::cerr << " [warn] index branch " << r.branchName + << " has non-signed-integer type — NOT remapped\n"; + continue; + } + resolved.push_back({found, r.perm}); + } + // Fill the output tree row by row in the requested order Long64_t nRemapped = 0; for (Long64_t srcRow : rowOrder) { src->GetEntry(srcRow); - // Remap the index branch if required - if (outIdxBr && idxTag != ScalarTag::kUnknown && !parentPerm.empty()) { - // Read old index from the input branch's buffer (one of the ios entries) - Long64_t oldIdx = -1; - for (auto &io : ios) { - if (io.desc.name == indexBranch) { oldIdx = readAsInt(io.dataBuf.data(), idxTag); break; } - } - Long64_t newIdx = -1; - if (oldIdx >= 0 && oldIdx < (Long64_t)parentPerm.size()) - newIdx = parentPerm[oldIdx]; - writeAsInt(newIdxBuf.data(), idxTag, newIdx >= 0 ? newIdx : -1); - if (newIdx != oldIdx) ++nRemapped; - } - - // Apply extra in-place index remaps (e.g. intra-table mother/daughter - // indices in O2mcparticle, or fIndexMcParticles in label tables). - // The output branch shares the same buffer, so modifying dataBuf here - // is read by out->Fill() below. - for (auto &er : extraRemaps) { - for (auto &io : ios) { - if (io.desc.name != er.branchName) continue; - if (io.desc.isVLA) { - // VLA: remap each element according to count - Long64_t cnt = readAsInt(io.countBuf.data(), io.countTag); - auto *p = reinterpret_cast(io.dataBuf.data()); - for (Long64_t j = 0; j < cnt; ++j) - p[j] = remapIdx(p[j], *er.perm); - } else { - // Scalar or fixed-size array: remap all nElems integers - auto *p = reinterpret_cast(io.dataBuf.data()); - for (int j = 0; j < io.desc.nElems; ++j) - p[j] = remapIdx(p[j], *er.perm); - } - break; - } + for (auto &r : resolved) { + BranchIO &io = *r.io; + int n = io.desc.isVLA ? (int)readAsInt(io.countBuf.data(), io.countTag) + : io.desc.nElems; + nRemapped += remapBuffer(io.dataBuf.data(), io.desc.tag, io.desc.elemSize, + n, *r.perm); } out->Fill(); @@ -497,9 +587,62 @@ static PermMap rewriteTable(TTree *src, TDirectory *dirOut, std::cout << " wrote " << out->GetEntries() << " / " << nSrc << " rows; " << nRemapped << " index values remapped\n"; out->Write(); - return srcToOut; } +// Look up the permutation of the table named by the first prefix that resolves. +// Ambiguity (several schema versions of the same table in one DF) is resolved +// deterministically by taking the lexicographically smallest name. +static const PermMap *findPermFor( + const std::unordered_map &allPerms, + const std::vector &prefixes, + std::string *foundName = nullptr) { + for (auto &prefix : prefixes) { + const PermMap *best = nullptr; + std::string bestName; + for (auto &[name, perm] : allPerms) { + if (!isTableNamed(name, prefix)) continue; + if (bestName.empty() || name < bestName) { best = &perm; bestName = name; } + } + if (best) { if (foundName) *foundName = bestName; return best; } + } + return nullptr; +} + +// THE central safety net: derive the complete remap list for a table from the +// kIndexRefs registry. Every stage writes through this, so an index column can +// only be missed by being absent from kIndexRefs — which +// unregisteredIndexBranches() reports. +static std::vector buildRemaps( + TTree *src, const std::unordered_map &allPerms) { + std::vector remaps; + if (!src) return remaps; + for (auto &[branchName, prefixes] : kIndexRefs) { + if (!src->GetBranch(branchName.c_str())) continue; + const PermMap *perm = findPermFor(allPerms, prefixes); + if (!perm) continue; // referent table not in this DF — nothing to remap + remaps.push_back({branchName, perm}); + } + return remaps; +} + +// ============================================================================ +// SECTION 3b — Deferred write plan +// ============================================================================ +// +// Writing is deferred until every table's row order (and hence its permutation) +// is known, because references can point forwards and sideways: O2fwdtrack +// references O2mfttrack — written later in the same stage — and also itself, via +// fIndexFwdTracks_MatchMCHTrack. The old code wrote each table as soon as it +// had planned it, so those two permutations simply did not exist yet at write +// time. That is the mechanism behind O2-7098. +// +// So: every stage now only PLANS (appends a TablePlan, publishes its perm into +// allPerms), and processDF writes all plans afterwards in one pass. +struct TablePlan { + std::string name; // tree name; re-fetched from dirIn at write time + std::vector rowOrder; // source rows to emit, in output order +}; + // ============================================================================ // SECTION 4 — Stage 0: BC table sort + deduplication // ============================================================================ @@ -527,7 +670,7 @@ struct BCStage0Result { Long64_t nUnique = 0; }; -static BCStage0Result stage0_sortBCs(TTree *treeBCs, TDirectory *dirOut) { +static BCStage0Result stage0_sortBCs(TTree *treeBCs, std::vector &plans) { BCStage0Result res; Long64_t n = treeBCs->GetEntries(); if (n == 0) return res; @@ -566,8 +709,8 @@ static BCStage0Result stage0_sortBCs(TTree *treeBCs, TDirectory *dirOut) { std::cout << " BC stage: " << n << " rows -> " << res.nUnique << " unique (sorted)\n"; - // Write the BC table (no index remapping needed for the table itself) - rewriteTable(treeBCs, dirOut, rowOrder, /*indexBranch=*/"", /*parentPerm=*/{}); + // The BC table itself carries no index columns. + plans.push_back({treeBCs->GetName(), std::move(rowOrder)}); return res; } @@ -576,10 +719,9 @@ static BCStage0Result stage0_sortBCs(TTree *treeBCs, TDirectory *dirOut) { // SECTION 5 — Stage 0b: BC flags table (follows BC row order exactly) // ============================================================================ -static void stage0_copyBCFlags(TTree *treeFlags, TDirectory *dirOut, +static void stage0_copyBCFlags(TTree *treeFlags, std::vector &plans, const PermMap &bcPerm) { if (!treeFlags) return; - Long64_t nSrc = treeFlags->GetEntries(); // Build rowOrder: for each unique output BC row, pick the first source row // that mapped to it @@ -590,7 +732,7 @@ static void stage0_copyBCFlags(TTree *treeFlags, TDirectory *dirOut, rowOrder.reserve(first.size()); for (auto &kv : first) rowOrder.push_back(kv.second); - rewriteTable(treeFlags, dirOut, rowOrder, /*indexBranch=*/"", /*parentPerm=*/{}); + plans.push_back({treeFlags->GetName(), std::move(rowOrder)}); } // ============================================================================ @@ -632,10 +774,10 @@ struct MCCollKeyHash { } }; -static std::unordered_map -stage1_BCindexedTables(TDirectory *dirIn, TDirectory *dirOut, +static void +stage1_BCindexedTables(TDirectory *dirIn, std::vector &plans, + std::unordered_map &allPerms, const PermMap &bcPerm) { - std::unordered_map tablePerms; TIter it(dirIn->GetListOfKeys()); while (TKey *key = static_cast(it())) { @@ -719,20 +861,19 @@ stage1_BCindexedTables(TDirectory *dirIn, TDirectory *dirOut, << " duplicate rows (" << rowOrder.size() << " kept)\n"; } - PermMap perm = rewriteTable(src, dirOut, rowOrder, idxBr, bcPerm); - tablePerms[tname] = std::move(perm); + allPerms[tname] = permFromRowOrder(nSrc, rowOrder); + plans.push_back({tname, std::move(rowOrder)}); } - return tablePerms; } // ============================================================================ // SECTION 7 — Stage 2: Tables indexed by MCCollisions // ============================================================================ -static std::unordered_map -stage2_MCCollIndexedTables(TDirectory *dirIn, TDirectory *dirOut, +static void +stage2_MCCollIndexedTables(TDirectory *dirIn, std::vector &plans, + std::unordered_map &allPerms, const PermMap &mcCollPerm) { - std::unordered_map tablePerms; TIter it(dirIn->GetListOfKeys()); while (TKey *key = static_cast(it())) { @@ -800,27 +941,16 @@ stage2_MCCollIndexedTables(TDirectory *dirIn, TDirectory *dirOut, std::cout << " dropped " << dropped << " rows whose MCCollision parent was deduplicated\n"; - // For O2mcparticle: compute the self-permutation (old row -> new row) from - // the row order BEFORE calling rewriteTable, then pass it as extra remaps - // so that intra-table mother/daughter indices are updated in the same pass. - // The stable sort above preserves within-collision particle order, which - // keeps fIndexSlice_Daughters contiguous — so remapping [first,last] via - // selfPerm is correct. - std::vector extraRemaps; - PermMap selfPerm; - if (TString(tname.c_str()).BeginsWith("O2mcparticle")) { - selfPerm.assign(nSrc, -1); - for (Long64_t outRow = 0; outRow < (Long64_t)rowOrder.size(); ++outRow) - selfPerm[rowOrder[outRow]] = (Int_t)outRow; - extraRemaps.push_back({"fIndexArray_Mothers", &selfPerm}); - extraRemaps.push_back({"fIndexSlice_Daughters", &selfPerm}); - std::cout << " O2mcparticle: will remap intra-table mother/daughter indices\n"; - } - - PermMap perm = rewriteTable(src, dirOut, rowOrder, idxBr, mcCollPerm, extraRemaps); - tablePerms[tname] = std::move(perm); + // O2mcparticle's intra-table mother/daughter links are remapped through this + // table's OWN permutation. No special case is needed any more: the perm is + // published here and buildRemaps() picks it up for fIndexArray_Mothers / + // fIndexSlice_Daughters at write time, because writing happens after all + // planning. The stable sort above preserves within-collision particle + // order, which keeps fIndexSlice_Daughters contiguous — so remapping + // [first,last] elementwise is correct. + allPerms[tname] = permFromRowOrder(nSrc, rowOrder); + plans.push_back({tname, std::move(rowOrder)}); } - return tablePerms; } // ============================================================================ @@ -835,46 +965,15 @@ stage2_MCCollIndexedTables(TDirectory *dirIn, TDirectory *dirOut, // // The list of paste-join pairs is in kPasteJoins (Section 1). // -// Index columns we know how to remap in a child: -// fIndexMcCollisions -> via mcCollPerm -// fIndexCollisions -> via collPerm -// fIndexMcParticles -> via mcParticlePerm -// fIndexArrayMcParticles -> via mcParticlePerm (VLA) +// This stage no longer enumerates which index columns it knows how to remap: +// buildRemaps() derives them from kIndexRefs at write time, for every table +// alike. The old hand-maintained enumeration here is what let O2fwdtrack slip +// through — the table was never routed to this function in the first place. // // When the named parent is not in allPerms (e.g. tracks aren't reordered in // this build), the child is processed with identity row order so the // value-wise remaps still apply but the row order is unchanged. -// Build the row order from a PermMap (srcRow -> outRow), inverted. -static std::vector rowOrderFromPerm(const PermMap &perm) { - // perm[srcRow] = outRow (or -1 if dropped) - // We need: outRow -> srcRow, i.e. a sorted list of (outRow, srcRow) pairs - std::vector> pairs; - pairs.reserve(perm.size()); - for (Long64_t srcRow = 0; srcRow < (Long64_t)perm.size(); ++srcRow) - if (perm[srcRow] >= 0) pairs.push_back({perm[srcRow], srcRow}); - std::sort(pairs.begin(), pairs.end()); - std::vector order; - order.reserve(pairs.size()); - for (auto &p : pairs) order.push_back(p.second); - return order; -} - -// Locate a permutation in allPerms whose key begins with a given prefix. -// Returns nullptr if none found. -static const PermMap *findPermByPrefix( - const std::unordered_map &allPerms, - const char *prefix, - std::string *foundName = nullptr) { - for (auto &[name, perm] : allPerms) { - if (TString(name.c_str()).BeginsWith(prefix)) { - if (foundName) *foundName = name; - return &perm; - } - } - return nullptr; -} - // ============================================================================ // SECTION 9b — Stage 1b: Collision-grouped track tables // ============================================================================ @@ -899,7 +998,14 @@ static const PermMap *findPermByPrefix( // * paste-join children (O2trackextra, O2trackcov_iu, O2mctracklabel, ...) // follow the published parent permutation; // * every fIndexTracks* / fIndexMFTTracks / fIndexFwdTracks reference is -// remapped through it in processPasteJoinTables. +// remapped through it — including the ones the track tables hold on EACH +// OTHER. O2fwdtrack points at O2mfttrack (fIndexMFTTracks) and at itself +// (fIndexFwdTracks_MatchMCHTrack), so its remap needs permutations that are +// only established later in this very stage. That is why this function +// PLANS all tables first and lets processDF write them afterwards; writing +// inline (as it used to) meant those two columns kept pre-reorder row +// numbers — in range, so no validator complained, but every global muon +// ended up with a foreign MFT and MCH leg. See O2-7098. static bool isCollGroupedTrackTable(const std::string &tname) { static const char *kPrefixes[] = {"O2track_iu", "O2track", "O2mfttrack", "O2fwdtrack"}; @@ -909,11 +1015,11 @@ static bool isCollGroupedTrackTable(const std::string &tname) { } static void stage1b_reorderTrackTables( - TDirectory *dirIn, TDirectory *dirOut, + TDirectory *dirIn, std::vector &plans, std::unordered_map &allPerms, - std::unordered_set &written) { + std::unordered_set &planned) { - const PermMap *collPermP = findPermByPrefix(allPerms, "O2collision_"); + const PermMap *collPermP = findPermFor(allPerms, {"O2collision"}); if (!collPermP) return; // no collisions present — nothing to regroup against TIter it(dirIn->GetListOfKeys()); @@ -924,7 +1030,7 @@ static void stage1b_reorderTrackTables( if (!src) continue; std::string tname = src->GetName(); - if (written.count(tname)) continue; // BC-indexed tracks etc. already done + if (planned.count(tname)) continue; // BC-indexed tracks etc. already done if (!isCollGroupedTrackTable(tname)) continue; if (isPasteJoinChild(tname)) continue; // children follow their parent below if (!src->GetBranch("fIndexCollisions")) continue; @@ -960,33 +1066,23 @@ static void stage1b_reorderTrackTables( rowOrder.reserve(nSrc); for (auto &e : entries) rowOrder.push_back(e.srcRow); - // Reorder rows and remap fIndexCollisions values through collPerm. - PermMap perm = rewriteTable(src, dirOut, rowOrder, "fIndexCollisions", *collPermP); - allPerms[tname] = std::move(perm); - written.insert(tname); + // Publish the permutation now; the actual write (with the full set of + // remaps, this table's own included) happens in processDF once every + // permutation in the DF is known. + allPerms[tname] = permFromRowOrder(nSrc, rowOrder); + plans.push_back({tname, std::move(rowOrder)}); + planned.insert(tname); } } -static void processPasteJoinTables( - TDirectory *dirIn, TDirectory *dirOut, - const std::unordered_map &allPerms, - const std::unordered_set &alreadyWritten, - const PermMap *bcPermP = nullptr) { - - // Pre-locate the parent permutations that paste-join children may want to - // apply to their own index columns. Any of these may legitimately be null - // (e.g. mcParticlePerm absent if there is no O2mcparticle in this DF). - const PermMap *mcParticlePerm = findPermByPrefix(allPerms, "O2mcparticle"); - const PermMap *mcCollPermP = findPermByPrefix(allPerms, "O2mccollision_"); - const PermMap *collPermP = findPermByPrefix(allPerms, "O2collision_"); - // Track tables reordered in Stage 1b: every reference into them must be - // remapped through their permutation (null if the table is absent / wasn't - // reordered, in which case no remap is needed). - const PermMap *trkPerm = findPermByPrefix(allPerms, "O2track_iu"); - const PermMap *mftPerm = findPermByPrefix(allPerms, "O2mfttrack"); - const PermMap *fwdPerm = findPermByPrefix(allPerms, "O2fwdtrack"); - // bcPermP is passed in from processDF (the BC table is the only stage - // whose permutation isn't already published in allPerms). +// Plan every table not yet claimed by an earlier stage: paste-join children +// follow their parent's row order, everything else keeps its own. The index +// columns they carry are NOT enumerated here any more — buildRemaps() derives +// them from kIndexRefs when processDF writes. +static void planRemainingTables( + TDirectory *dirIn, std::vector &plans, + std::unordered_map &allPerms, + std::unordered_set &planned) { TIter it(dirIn->GetListOfKeys()); while (TKey *key = static_cast(it())) { @@ -996,100 +1092,56 @@ static void processPasteJoinTables( if (!src) continue; std::string tname = src->GetName(); - if (alreadyWritten.count(tname)) continue; + if (planned.count(tname)) continue; if (isBCTable(tname.c_str())) continue; // Stage-1 BC-indexed and Stage-2 MCColl-indexed non-paste-join tables are - // already in alreadyWritten. A paste-join child carrying its own MCColl - // index (e.g. O2mccollisionlabel) was deferred from stage2 and lands here. + // already planned. A paste-join child carrying its own MCColl index + // (e.g. O2mccollisionlabel) was deferred from stage2 and lands here. if (bcIndexBranch(src)) continue; if (mcCollIndexBranch(src) && !isPasteJoinChild(tname)) continue; - // Build value-wise extra remaps for any index column this table carries - // that points into a table whose row order may have changed. - std::vector extraRemaps; - if (mcParticlePerm) { - if (src->GetBranch("fIndexMcParticles")) - extraRemaps.push_back({"fIndexMcParticles", mcParticlePerm}); - if (src->GetBranch("fIndexArrayMcParticles")) - extraRemaps.push_back({"fIndexArrayMcParticles", mcParticlePerm}); - } - if (mcCollPermP && src->GetBranch("fIndexMcCollisions")) - extraRemaps.push_back({"fIndexMcCollisions", mcCollPermP}); - if (collPermP && src->GetBranch("fIndexCollisions")) - extraRemaps.push_back({"fIndexCollisions", collPermP}); - if (bcPermP) { - // BC-pointing indices that weren't already remapped in Stage 1. - // Stage-1 BC-indexed tables (with fIndexBCs / fIndexBC) are in - // alreadyWritten by now, so this only fires for tables that escaped - // Stage 1 — chiefly the O2ambiguous* family, which carries the SOA - // SLICE_INDEX_COLUMN(BC, bc) stored on disk as fIndexSliceBCs[2]/I. - // After BC dedup the slice endpoints would otherwise point past the - // compacted BC table; remapping through bcPerm fixes this. - if (src->GetBranch("fIndexSliceBCs")) - extraRemaps.push_back({"fIndexSliceBCs", bcPermP}); - if (src->GetBranch("fIndexBCs")) - extraRemaps.push_back({"fIndexBCs", bcPermP}); - if (src->GetBranch("fIndexBC")) - extraRemaps.push_back({"fIndexBC", bcPermP}); - } - - // Track-pointing indices: the track tables may have been reordered in - // Stage 1b, so every reference into them must be remapped through the - // corresponding permutation. (No-op when the perm is null / absent.) - auto addTrkRemap = [&](const char *br, const PermMap *pm) { - if (pm && src->GetBranch(br)) extraRemaps.push_back({br, pm}); - }; - addTrkRemap("fIndexTracks", trkPerm); - addTrkRemap("fIndexTracks_0", trkPerm); - addTrkRemap("fIndexTracks_1", trkPerm); - addTrkRemap("fIndexTracks_2", trkPerm); - addTrkRemap("fIndexTracks_Pos", trkPerm); - addTrkRemap("fIndexTracks_Neg", trkPerm); - addTrkRemap("fIndexTracks_ITS", trkPerm); - addTrkRemap("fIndexMFTTracks", mftPerm); - addTrkRemap("fIndexFwdTracks", fwdPerm); - addTrkRemap("fIndexFwdTracks_MatchMCHTrack", fwdPerm); + Long64_t nSrc = src->GetEntries(); // Find a paste-join parent for this table (kPasteJoins lookup). const PermMap *parentPerm = nullptr; std::string parentName; for (auto &[pastePrefix, parentPrefix] : kPasteJoins) { if (!TString(tname.c_str()).BeginsWith(pastePrefix.c_str())) continue; - parentPerm = findPermByPrefix(allPerms, parentPrefix.c_str(), &parentName); + parentPerm = findPermFor(allPerms, {parentPrefix}, &parentName); if (parentPerm) break; } + std::vector rowOrder; if (parentPerm) { + // perm[srcRow] = outRow (-1 = dropped); invert it into an output order. + std::vector> pairs; + pairs.reserve(parentPerm->size()); + for (Long64_t srcRow = 0; srcRow < (Long64_t)parentPerm->size(); ++srcRow) + if ((*parentPerm)[srcRow] >= 0) pairs.push_back({(*parentPerm)[srcRow], srcRow}); + std::sort(pairs.begin(), pairs.end()); + rowOrder.reserve(pairs.size()); + for (auto &p : pairs) rowOrder.push_back(p.second); + } + + if (parentPerm && (Long64_t)rowOrder.size() == nSrc) { std::cout << " Paste-join: " << tname << " follows " << parentName << "\n"; - auto rowOrder = rowOrderFromPerm(*parentPerm); - if ((Long64_t)rowOrder.size() != src->GetEntries()) { + } else { + if (parentPerm) { + // Schema drift: the child cannot be aligned to its parent. Keep its own + // row order (the index remaps still get applied at write time, so at + // least no value is left dangling) and shout — the validator's + // paste-join parity check will fail on the output. std::cerr << " [warn] paste-join size mismatch: " << tname - << " has " << src->GetEntries() << " rows but parent perm covers " - << rowOrder.size() << " — cloning as-is\n"; - dirOut->cd(); - TTree *c = src->CloneTree(-1, "fast"); - c->SetDirectory(dirOut); - c->Write(); - } else { - rewriteTable(src, dirOut, rowOrder, "", {}, extraRemaps); + << " has " << nSrc << " rows but parent perm covers " + << rowOrder.size() << " — keeping own row order\n"; } - } else if (!extraRemaps.empty() || isPasteJoinChild(tname)) { - // Parent wasn't reordered (or not present in this DF) — keep row order - // identical but still apply value-wise index remaps and follow the - // paste-join 1:1 invariant by going through the identity row order. - std::cout << " Identity-order remap: " << tname << "\n"; - Long64_t n = src->GetEntries(); - std::vector identity(n); - std::iota(identity.begin(), identity.end(), 0LL); - rewriteTable(src, dirOut, identity, "", {}, extraRemaps); - } else { - // No paste-join and no index remapping needed — fast clone - std::cout << " Copy (no dependency): " << tname << "\n"; - dirOut->cd(); - TTree *c = src->CloneTree(-1, "fast"); - c->SetDirectory(dirOut); - c->Write(); + rowOrder.resize(nSrc); + std::iota(rowOrder.begin(), rowOrder.end(), 0LL); } + + allPerms[tname] = permFromRowOrder(nSrc, rowOrder); + plans.push_back({tname, std::move(rowOrder)}); + planned.insert(tname); } } @@ -1152,52 +1204,86 @@ static void processDF(TDirectory *dirIn, TDirectory *dirOut) { return; } + // ========================================================================== + // PLAN PHASE — every stage only decides row orders and publishes the + // resulting permutations. Nothing is written yet, because a table's index + // columns may point at tables planned later in the same or a later stage + // (O2fwdtrack -> O2mfttrack, and O2fwdtrack -> itself). + // ========================================================================== + std::vector plans; + std::unordered_map allPerms; + std::unordered_set planned; + // ---- Stage 0: sort & deduplicate BCs ---- std::cout << "-- Stage 0: BCs --\n"; - dirOut->cd(); - BCStage0Result s0 = stage0_sortBCs(treeBCs, dirOut); - if (treeFlags) stage0_copyBCFlags(treeFlags, dirOut, s0.bcPerm); - - // Track which tree names have been written so we don't double-write - std::unordered_set written; - written.insert(treeBCs->GetName()); - if (treeFlags) written.insert(treeFlags->GetName()); + BCStage0Result s0 = stage0_sortBCs(treeBCs, plans); + if (treeFlags) stage0_copyBCFlags(treeFlags, plans, s0.bcPerm); + planned.insert(treeBCs->GetName()); + if (treeFlags) planned.insert(treeFlags->GetName()); + // bcPerm is a dedup map (several old rows collapse onto one new row), not a + // plain row permutation — which is exactly the mapping references into the BC + // table need. Publishing it under the BC tree's real name lets buildRemaps() + // resolve fIndexBCs / fIndexBC / fIndexSliceBCs like any other reference. + allPerms[treeBCs->GetName()] = s0.bcPerm; // ---- Stage 1: BC-indexed tables (including MCCollisions dedup) ---- std::cout << "-- Stage 1: BC-indexed tables --\n"; - auto stage1Perms = stage1_BCindexedTables(dirIn, dirOut, s0.bcPerm); - for (auto &kv : stage1Perms) written.insert(kv.first); + stage1_BCindexedTables(dirIn, plans, allPerms, s0.bcPerm); + for (auto &kv : allPerms) planned.insert(kv.first); // ---- Stage 2: MCCollision-indexed tables ---- - // Find the MCCollision permutation from stage 1 std::cout << "-- Stage 2: MCCollision-indexed tables --\n"; - PermMap mcCollPerm; - for (auto &[tname, perm] : stage1Perms) { - if (TString(tname.c_str()).BeginsWith("O2mccollision")) { - mcCollPerm = perm; - break; - } - } - if (!mcCollPerm.empty()) { - auto stage2Perms = stage2_MCCollIndexedTables(dirIn, dirOut, mcCollPerm); - for (auto &kv : stage2Perms) { - written.insert(kv.first); - stage1Perms[kv.first] = kv.second; // merge into allPerms for paste-join lookup - } + const PermMap *mcCollPermP = findPermFor(allPerms, {"O2mccollision"}); + if (mcCollPermP) { + PermMap mcCollPerm = *mcCollPermP; // copy: allPerms grows below + stage2_MCCollIndexedTables(dirIn, plans, allPerms, mcCollPerm); + for (auto &kv : allPerms) planned.insert(kv.first); } else { std::cout << " (no MCCollision table found — skipping stage 2)\n"; } // ---- Stage 1b: regroup collision-grouped track tables ---- - // Must run after Stage 1 (needs the collision permutation) and before the - // paste-join stage (so children follow the new track order and fIndexTracks* - // references are remapped). Publishes track permutations into stage1Perms. + // Must run after Stage 1 (needs the collision permutation). std::cout << "-- Stage 1b: collision-grouped track tables --\n"; - stage1b_reorderTrackTables(dirIn, dirOut, stage1Perms, written); + stage1b_reorderTrackTables(dirIn, plans, allPerms, planned); // ---- Paste-join tables + unrelated tables ---- std::cout << "-- Paste-join and unrelated tables --\n"; - processPasteJoinTables(dirIn, dirOut, stage1Perms, written, &s0.bcPerm); + planRemainingTables(dirIn, plans, allPerms, planned); + + // ========================================================================== + // WRITE PHASE — all permutations are known, so every table can now have + // ALL of its index columns remapped, whichever table they point at. + // ========================================================================== + std::cout << "-- Writing " << plans.size() << " tables --\n"; + for (auto &plan : plans) { + TTree *src = dynamic_cast(dirIn->Get(plan.name.c_str())); + if (!src) { std::cerr << " [warn] lost tree " << plan.name << " before write\n"; continue; } + + // Drift guard: a link the registry does not know about cannot be remapped, + // so say so loudly here as well as in the validator. + for (auto &b : unregisteredIndexBranches(src)) + std::cerr << " [warn] " << plan.name << "." << b + << " is not in kIndexRefs — it will NOT be remapped\n"; + + auto remaps = buildRemaps(src, allPerms); + + bool identity = ((Long64_t)plan.rowOrder.size() == src->GetEntries()); + for (Long64_t i = 0; identity && i < (Long64_t)plan.rowOrder.size(); ++i) + if (plan.rowOrder[i] != i) identity = false; + + if (identity && remaps.empty()) { + std::cout << " Copy (no dependency): " << plan.name << "\n"; + dirOut->cd(); + TTree *c = src->CloneTree(-1, "fast"); + c->SetDirectory(dirOut); + c->Write(); + continue; + } + + std::cout << " Write: " << plan.name << " (" << remaps.size() << " index column(s))\n"; + writeTable(src, dirOut, plan.rowOrder, remaps); + } // ---- Non-tree objects (TMap metadata) ---- copyNonTreeObjects(dirIn, dirOut); @@ -1217,46 +1303,23 @@ static void processDF(TDirectory *dirIn, TDirectory *dirOut) { // (e.g. O2mccollisionlabel matches O2collision). // 4. Every fIndex* value across the DF is in range w.r.t. its referent // table (value -1 is always permitted as the "no link" sentinel). +// 5. Every fIndex* branch present is covered by kIndexRefs — an unknown link +// is a link nothing remapped. // // Returns true if all checks pass. Prints [FAIL] lines for each violation. +// +// NOTE on what these checks can and cannot see: they are all *structural*. The +// O2-7098 corruption was in-range and structurally perfect — the row numbers +// were simply the wrong ones. Catching that needs the input file too; see +// AODBcRewriterCheckLinks() at the end of this section. -// Map from fIndex* branch name to the table-name prefix it refers to. The -// match on the referent side uses TString::BeginsWith so versioned suffixes -// (O2collision_001, O2bc_001, ...) are handled. Branches not in this list -// are skipped by the range check (this includes O2mcparticle's intra-table -// fIndexArray_Mothers / fIndexSlice_Daughters, which are checked separately -// with stricter semantics in the MC-particle block). -static const std::vector> kIndexBranchToTable = { - { "fIndexBCs", "O2bc_" }, - { "fIndexBC", "O2bc_" }, - { "fIndexSliceBCs", "O2bc_" }, - { "fIndexCollisions", "O2collision_" }, - { "fIndexCollision", "O2collision_" }, - { "fIndexMcCollisions", "O2mccollision_" }, - { "fIndexMcParticles", "O2mcparticle" }, - { "fIndexArrayMcParticles", "O2mcparticle" }, - { "fIndexTracks", "O2track_iu" }, - { "fIndexTracks_0", "O2track_iu" }, - { "fIndexTracks_1", "O2track_iu" }, - { "fIndexTracks_2", "O2track_iu" }, - { "fIndexTracks_Pos", "O2track_iu" }, - { "fIndexTracks_Neg", "O2track_iu" }, - { "fIndexTracks_ITS", "O2track_iu" }, - { "fIndexFwdTracks", "O2fwdtrack" }, - { "fIndexFwdTracks_MatchMCHTrack", "O2fwdtrack" }, - { "fIndexMFTTracks", "O2mfttrack" }, - { "fIndexV0s", "O2v0_" }, - { "fIndexCascades", "O2cascade_" }, - { "fIndexDecay3Bodys", "O2decay3body" }, -}; - -// Find a tree in d whose name begins with the given prefix. Returns the -// number of entries, or -1 if not found. +// Find a tree in d whose name matches the given table prefix (see isTableNamed). +// Returns the number of entries, or -1 if not found. static Long64_t treeEntriesByPrefix(TDirectory *d, const char *prefix) { TIter it(d->GetListOfKeys()); TKey *k; while ((k = (TKey*)it())) { - if (!TString(k->GetName()).BeginsWith(prefix)) continue; + if (!isTableNamed(k->GetName(), prefix)) continue; TObject *obj = d->Get(k->GetName()); if (!obj || !obj->InheritsFrom(TTree::Class())) continue; return ((TTree*)obj)->GetEntries(); @@ -1264,6 +1327,17 @@ static Long64_t treeEntriesByPrefix(TDirectory *d, const char *prefix) { return -1; } +// Entries of the table a given index branch refers to (first prefix that +// resolves wins), or -1 if the referent is not in this DF. +static Long64_t referentEntries(TDirectory *d, + const std::vector &prefixes) { + for (auto &p : prefixes) { + Long64_t n = treeEntriesByPrefix(d, p.c_str()); + if (n >= 0) return n; + } + return -1; +} + // Generic in-range check for every fIndex* branch listed above. Reads the // branch's leaf (scalar, fixed-array, or VLA), iterates all entries, and // counts how many values are outside [-1, nReferent). @@ -1439,19 +1513,29 @@ static bool validateDF(TDirectory *d) { } } - // ---- Generic fIndex* range check ---- + // ---- Generic fIndex* range check + registry drift guard ---- // For each table in the DF, scan all fIndex* branches and confirm every // value lies in [-1, nReferent). This catches stale pointers across // tables (cross-table index drift) which a per-DF-tree-only check misses. + // + // Any fIndex* branch that is NOT in kIndexRefs is a hard failure: the + // rewriter cannot have remapped it, so if the referent table was reordered + // the column is now silently wrong. Adding the entry to kIndexRefs is the + // fix — that is the whole point of having one registry. TIter it2(d->GetListOfKeys()); TKey *k2; while ((k2 = (TKey*)it2())) { TObject *obj = d->Get(k2->GetName()); if (!obj || !obj->InheritsFrom(TTree::Class())) continue; TTree *t = (TTree*)obj; - for (auto &[branchName, referentPrefix] : kIndexBranchToTable) { + for (auto &b : unregisteredIndexBranches(t)) { + std::cerr << " [FAIL] " << t->GetName() << "." << b + << " is not registered in kIndexRefs — nothing remaps it\n"; + ok = false; + } + for (auto &[branchName, referentPrefixes] : kIndexRefs) { if (!t->GetBranch(branchName.c_str())) continue; - Long64_t nRef = treeEntriesByPrefix(d, referentPrefix.c_str()); + Long64_t nRef = referentEntries(d, referentPrefixes); if (nRef < 0) continue; // referent not in this DF; skip silently Long64_t bad = checkIndexRange(t, branchName.c_str(), nRef); if (bad > 0) { @@ -1513,6 +1597,294 @@ bool AODBcRewriterValidate(const char *fname = "AO2D_rewritten.root") { return allOk; } +// ============================================================================ +// SECTION 11b — Link preservation check (input vs output) +// ============================================================================ +// +// WHY THIS EXISTS +// +// Everything in Section 11 is a *structural* check of the output alone: are the +// BCs monotonic, are the paste-join row counts equal, is every index value in +// range. O2-7098 passed all of them. Stage 1b reordered O2fwdtrack without +// remapping fIndexMFTTracks / fIndexFwdTracks_MatchMCHTrack, so every global +// muon pointed at a perfectly valid, perfectly in-range, completely unrelated +// MFT track. No structural check can see that — you have to compare against +// the input. +// +// THE INVARIANT +// +// The rewriter permutes rows and drops duplicates. It must never change what a +// row *is* or what it *points at*. So for every table, the multiset of tuples +// +// ( payload fingerprint of the row, +// payload fingerprint of the row each index column points at, ... ) +// +// must be the same before and after — up to rows that were legitimately +// dropped. Row identity comes from the payload fingerprint (a hash of every +// non-fIndex* branch), so no permutation map, no synthetic tagging and no +// assumption about ordering is needed. It runs on real production AO2Ds. +// +// Dedup is handled by canonicalising the INPUT side: if a referenced input row +// has no counterpart in the output (it was dropped), the link is rewritten to +// the "no link" sentinel first, which is exactly what the rewriter does. A +// tuple that exists in the output but in no input row is then always a bug. + +static const ULong64_t kFnvOffset = 14695981039346656037ULL; +static const ULong64_t kFnvPrime = 1099511628211ULL; +static const ULong64_t kNullFp = 0ULL; // reserved: "points at nothing" + +static ULong64_t fnv1a(ULong64_t h, const void *data, size_t n) { + const unsigned char *p = static_cast(data); + for (size_t i = 0; i < n; ++i) { h ^= p[i]; h *= kFnvPrime; } + return h; +} + +// Per-row hash of every non-index branch. Two rows with the same fingerprint +// are interchangeable as far as this check is concerned, which is the right +// semantics: identical rows may be freely permuted among themselves. +// +// `onlyBranch`, when given, restricts the hash to that single branch. It is +// used for the BC table, and only for it: Stage 0 deduplicates BCs by +// fGlobalBC and *redirects* references onto the surviving row rather than +// nulling them, so two BC rows that share a fGlobalBC have to be treated as one +// identity here. Every other dedup in this tool drops rows and nulls the +// references, which the survivor canonicalisation below handles. +static std::vector payloadFingerprints(TTree *t, + const char *onlyBranch = nullptr) { + std::vector fps; + if (!t) return fps; + Long64_t n = t->GetEntries(); + fps.resize(n, kFnvOffset); + + auto descs = describeBranches(t); + struct IO { + BranchDesc desc; + std::vector dataBuf, countBuf; + ScalarTag countTag = ScalarTag::kUnknown; + TBranch *br = nullptr, *cntBr = nullptr; + }; + std::vector ios; + for (auto &d : descs) { + if (onlyBranch) { if (d.name != onlyBranch) continue; } + else if (d.name.rfind("fIndex", 0) == 0) continue; // index columns are not payload + IO io; io.desc = d; + io.dataBuf.assign((d.isVLA ? d.maxElems : d.nElems) * d.elemSize, 0); + io.br = t->GetBranch(d.name.c_str()); + if (d.isVLA) { + io.cntBr = t->GetBranch(d.countBranchName.c_str()); + TLeaf *cl = io.cntBr ? static_cast(io.cntBr->GetListOfLeaves()->At(0)) : nullptr; + io.countTag = cl ? tagOf(cl) : ScalarTag::kUnknown; + io.countBuf.assign(byteSize(io.countTag), 0); + } + ios.push_back(std::move(io)); + } + for (auto &io : ios) { + if (io.br) io.br->SetAddress(io.dataBuf.data()); + if (io.cntBr) io.cntBr->SetAddress(io.countBuf.data()); + } + + for (Long64_t i = 0; i < n; ++i) { + ULong64_t h = kFnvOffset; + for (auto &io : ios) { + if (io.br) io.br->GetEntry(i); + if (io.cntBr) io.cntBr->GetEntry(i); + if (io.desc.isVLA) { + Long64_t cnt = readAsInt(io.countBuf.data(), io.countTag); + h = fnv1a(h, &cnt, sizeof(cnt)); + h = fnv1a(h, io.dataBuf.data(), (size_t)std::max(0, cnt) * io.desc.elemSize); + } else { + h = fnv1a(h, io.dataBuf.data(), (size_t)io.desc.nElems * io.desc.elemSize); + } + } + fps[i] = (h == kNullFp) ? 1ULL : h; // never collide with the NULL sentinel + } + t->ResetBranchAddresses(); + return fps; +} + +// One index column of a table, resolved to the referent's fingerprint table. +struct LinkColumn { + std::string branchName; + const std::vector *refFp = nullptr; // referent fingerprints, same file + const std::unordered_set *survivors = nullptr; // input side only +}; + +// Multiset (as a count map) of the per-row tuples described at the top of this +// section, hashed down to a single 64-bit key. +static std::map linkTupleCounts( + TTree *t, const std::vector &ownFp, + const std::vector &links) { + + std::map counts; + Long64_t n = t->GetEntries(); + + struct IO { + BranchDesc desc; + std::vector dataBuf, countBuf; + ScalarTag countTag = ScalarTag::kUnknown; + TBranch *br = nullptr, *cntBr = nullptr; + const LinkColumn *link = nullptr; + }; + auto descs = describeBranches(t); + std::vector ios; + for (auto &lk : links) { + for (auto &d : descs) { + if (d.name != lk.branchName) continue; + IO io; io.desc = d; io.link = &lk; + io.dataBuf.assign((d.isVLA ? d.maxElems : d.nElems) * d.elemSize, 0); + io.br = t->GetBranch(d.name.c_str()); + if (d.isVLA) { + io.cntBr = t->GetBranch(d.countBranchName.c_str()); + TLeaf *cl = io.cntBr ? static_cast(io.cntBr->GetListOfLeaves()->At(0)) : nullptr; + io.countTag = cl ? tagOf(cl) : ScalarTag::kUnknown; + io.countBuf.assign(byteSize(io.countTag), 0); + } + ios.push_back(std::move(io)); + break; + } + } + for (auto &io : ios) { + if (io.br) io.br->SetAddress(io.dataBuf.data()); + if (io.cntBr) io.cntBr->SetAddress(io.countBuf.data()); + } + + for (Long64_t i = 0; i < n; ++i) { + ULong64_t h = fnv1a(kFnvOffset, &ownFp[i], sizeof(ULong64_t)); + for (auto &io : ios) { + if (io.br) io.br->GetEntry(i); + if (io.cntBr) io.cntBr->GetEntry(i); + int cnt = io.desc.isVLA ? (int)readAsInt(io.countBuf.data(), io.countTag) + : io.desc.nElems; + for (int j = 0; j < cnt; ++j) { + Long64_t v = readAsInt(io.dataBuf.data() + (size_t)j * io.desc.elemSize, + io.desc.tag); + ULong64_t fp = kNullFp; + if (v >= 0 && v < (Long64_t)io.link->refFp->size()) { + fp = (*io.link->refFp)[v]; + // Input side: a referent row that did not survive into the output is + // expected to become a null link, so canonicalise it to one here. + if (io.link->survivors && !io.link->survivors->count(fp)) fp = kNullFp; + } + h = fnv1a(h, &fp, sizeof(fp)); + } + } + ++counts[h]; + } + t->ResetBranchAddresses(); + return counts; +} + +static bool checkLinksDF(TDirectory *din, TDirectory *dout, const char *dfName) { + bool ok = true; + + // Collect the tables present in both files. + std::vector tables; + TIter it(din->GetListOfKeys()); + while (TKey *k = static_cast(it())) { + if (TString(k->GetClassName()) != "TTree") continue; + std::string tn = k->GetName(); + if (!dout->Get(tn.c_str())) { + std::cerr << " [FAIL] " << dfName << ": table " << tn << " missing from output\n"; + ok = false; + continue; + } + tables.push_back(tn); + } + + // Fingerprint every table in both files once — referents are looked up by name. + std::unordered_map> fpIn, fpOut; + std::unordered_map> survivors; + for (auto &tn : tables) { + const char *only = isTableNamed(tn, "O2bc") ? "fGlobalBC" : nullptr; + fpIn[tn] = payloadFingerprints(dynamic_cast(din->Get(tn.c_str())), only); + fpOut[tn] = payloadFingerprints(dynamic_cast(dout->Get(tn.c_str())), only); + survivors[tn].insert(fpOut[tn].begin(), fpOut[tn].end()); + } + + for (auto &tn : tables) { + TTree *tIn = dynamic_cast(din->Get(tn.c_str())); + TTree *tOut = dynamic_cast(dout->Get(tn.c_str())); + if (!tIn || !tOut) continue; + + if (tOut->GetEntries() > tIn->GetEntries()) { + std::cerr << " [FAIL] " << dfName << ": " << tn << " grew from " + << tIn->GetEntries() << " to " << tOut->GetEntries() << " rows\n"; + ok = false; + } + + // Resolve this table's index columns to their referent fingerprint tables. + std::vector linksIn, linksOut; + for (auto &[branchName, prefixes] : kIndexRefs) { + if (!tIn->GetBranch(branchName.c_str())) continue; + std::string refName; + for (auto &p : prefixes) { + for (auto &cand : tables) if (isTableNamed(cand, p)) { refName = cand; break; } + if (!refName.empty()) break; + } + if (refName.empty()) continue; // referent not in this DF + linksIn.push_back({branchName, &fpIn[refName], &survivors[refName]}); + linksOut.push_back({branchName, &fpOut[refName], nullptr}); + } + + auto cIn = linkTupleCounts(tIn, fpIn[tn], linksIn); + auto cOut = linkTupleCounts(tOut, fpOut[tn], linksOut); + + // Every output tuple must be accounted for by an input tuple. The reverse + // is not required: dedup legitimately removes rows. + Long64_t unexplained = 0; + for (auto &[key, nOut] : cOut) { + auto found = cIn.find(key); + Long64_t nIn = (found == cIn.end()) ? 0 : found->second; + if (nOut > nIn) unexplained += nOut - nIn; + } + if (unexplained > 0) { + std::cerr << " [FAIL] " << dfName << ": " << tn << " — " << unexplained + << " of " << tOut->GetEntries() + << " output row(s) have a payload/link combination that no input" + " row had (rows or references were mis-permuted)\n"; + ok = false; + } + } + return ok; +} + +// Compare a rewritten AO2D against the file it was produced from and verify +// that no row changed what it points at. This is the check that catches the +// O2-7098 bug class; run it whenever AODBcRewriter.C is touched. +bool AODBcRewriterCheckLinks(const char *inFileName = "AO2D_pre.root", + const char *outFileName = "AO2D_rewritten.root") { + std::cout << "Checking link preservation: " << inFileName + << " -> " << outFileName << "\n"; + if (TString(inFileName).BeginsWith("alien:") || + TString(outFileName).BeginsWith("alien:")) TGrid::Connect("alien"); + std::unique_ptr fin(TFile::Open(inFileName, "READ")); + std::unique_ptr fout(TFile::Open(outFileName, "READ")); + if (!fin || fin->IsZombie()) { std::cerr << "Cannot open " << inFileName << "\n"; return false; } + if (!fout || fout->IsZombie()) { std::cerr << "Cannot open " << outFileName << "\n"; return false; } + + bool allOk = true; + int nDF = 0; + TIter top(fin->GetListOfKeys()); + while (TKey *k = static_cast(top())) { + if (!isDF(k->GetName())) continue; + TDirectory *din = dynamic_cast(fin->Get(k->GetName())); + TDirectory *dout = dynamic_cast(fout->Get(k->GetName())); + if (!din) continue; + if (!dout) { + std::cerr << " [FAIL] " << k->GetName() << " missing from output\n"; + allOk = false; + continue; + } + allOk = checkLinksDF(din, dout, k->GetName()) && allOk; + ++nDF; + } + fin->Close(); + fout->Close(); + if (allOk) std::cout << "LINK CHECK PASSED (" << nDF << " DFs checked)\n"; + else std::cout << "LINK CHECK FAILED — see [FAIL] lines above\n"; + return allOk; +} + // ============================================================================ // SECTION 12 — Top-level entry point // ============================================================================ diff --git a/MC/utils/CLAUDE.md b/MC/utils/CLAUDE.md index 56968ed16..52bc83901 100644 --- a/MC/utils/CLAUDE.md +++ b/MC/utils/CLAUDE.md @@ -1,11 +1,59 @@ # CLAUDE.md — AODBcRewriter Development Handoff -## Current work state (handoff — 2026-06-09) - -**Branch:** `fix/aodbcrewriter-track-regroup` · **Last commit:** `8bb9d30b3c` -(committed, not yet pushed/validated). - -### The bug being fixed: tracks' `-1` collision group split +## Current work state (2026-07-28) — O2-7098 + +**Branch:** `swenzel/O2-7098-aodbcrewriter-index-remap`. + +### The bug: O2fwdtrack's match indices were reordered but not remapped + +Reported by Maurice Coquet (JIRA **O2-7098**, patch proposal +[O2DPG#2418](https://github.com/AliceO2Group/O2DPG/pull/2418)): in anchored MC +AO2Ds produced since early June 2026, `O2fwdtrack.fIndexMFTTracks` and +`O2fwdtrack.fIndexFwdTracks_MatchMCHTrack` point at the wrong rows, so the MFT +leg and the MCH leg of every global muon belong to different MC particles +(`sameParticle=0`). + +**Cause.** Stage 1b (added by #2370, merged 4 Jun 2026) re-sorts `O2track_iu`, +`O2mfttrack_*` and `O2fwdtrack`, but wrote each table the moment it had planned +it. `O2fwdtrack` points at `O2mfttrack` — reordered later in the same loop — and +at *itself*; neither permutation existed yet at write time, so the two columns +kept pre-reorder row numbers. They stayed in range, so every structural check +passed. + +**Scope.** The corruption does *not* need the BC→collision reorder cascade. +Stage 1b re-sorts unconditionally to make the split `-1` group contiguous, so in +practice **every merged MC AO2D since 4 Jun 2026 is affected**. + +**Reproduced** on `example_AOD/AO2D_pre.root`: 171 fwd tracks with an MFT match, +`sameParticle` 30 → **0** with the pre-fix code, 30 → **30** with the fix. + +### What the fix does + +1. **One index registry, `kIndexRefs` (Section 1).** Every `fIndex*` column and + the table it points at, in one place, used by the rewriter, the validator and + the drift guard alike. Referent resolution is `isTableNamed()` (`K == P` or + `K == P_`), which — unlike the old `BeginsWith` — keeps + `O2mfttrack_001` apart from `O2mfttrackcov`, `O2bc_001` from `O2bcflag` and + `O2calo` from `O2calotrigger`. +2. **`buildRemaps()` (Section 3)** derives a table's complete remap list from + that registry. The privileged "primary index + optional extras" split in + `rewriteTable` is gone — there is one `writeTable(src, dirOut, rowOrder, + remaps)` and every column goes through the same path. Forgetting a column is + no longer possible; the old design *required* each stage to remember. +3. **Plan, then write (Sections 3b + 10).** Every stage now only appends a + `TablePlan` and publishes its permutation; `processDF` writes all plans + afterwards, when every permutation in the DF is known. This is what makes the + forward reference (fwd→mft) and the self reference (fwd→fwd) work. +4. **Drift guard.** Any `fIndex*` branch not in `kIndexRefs` is a `[warn]` at + rewrite time and a `[FAIL]` in the validator. Schema growth now breaks the + test instead of silently mis-linking data. +5. **`AODBcRewriterCheckLinks()` (Section 11b)** — the check that can actually + see this bug class; see "Testing" below. + +Maurice's #2418 has the same shape (defer `O2fwdtrack`, remap via the MFT/Fwd +perms) and is correct; this generalises it from one table to all of them. + +### Previous work state (2026-06-09): tracks' `-1` collision group split Downstream O2 analysis (`o2-analysis-event-selection`) was crashing with: @@ -51,24 +99,71 @@ and forces a reorder cascade **BC (Stage 0) → collisions (Stage 1) → tracks **not** re-introduce an "assert already sorted / order-preserving" Stage 0 — it is a known dead end (see the history note in the Section 4 code comment). -### Not done yet / next steps for whoever picks this up - -- **UNVALIDATED on real data.** Parses cleanly in cling (`.L AODBcRewriter.C`), - but has *not* been run on a real merged AO2D, nor through the analysis task - that crashed. The macro is interpreter-only; `.L AODBcRewriter.C+` (ACLiC) - fails on missing std includes — pre-existing, not a regression. - Test sequence: `AODBcRewriter("AO2D.root","out.root")` → - `AODBcRewriterValidate("out.root")` (expect no `[FAIL]`) → then the **real** - `o2-analysis-event-selection` on `out.root` (ground truth). -- **Known fragility (whack-a-mole):** the `fIndexTracks*` reference remap is a - hardcoded enumeration in `processPasteJoinTables` and the validator's - `kIndexBranchToTable`. A missed reference into a reordered track table = silent - corruption. Longer term, *derive* index→referent relationships from the AO2D - column-name conventions instead of enumerating. -- **Biggest gap:** there is no executable analysis-level CI for this tool, so - regressions are only found in production with delay. Building a reproducer - (merged AO2D that triggers the split/abort) + a CI check that runs the real - task is the agreed top priority after this fix lands. +### Status of the 2026-06-09 items + +- ~~**UNVALIDATED on real data.**~~ Now run on `example_AOD/AO2D_pre.root`, + `bigger/` and `bigger2/` (3–4 DFs each, up to 6M MC particles): rewrite clean, + `AODBcRewriterValidate` and `AODBcRewriterCheckLinks` both pass. The macro is + still interpreter-only; `.L AODBcRewriter.C+` (ACLiC) fails on missing std + includes — pre-existing, not a regression. +- ~~**Known fragility (whack-a-mole).**~~ Resolved by `kIndexRefs` + + `buildRemaps()` + the drift guard (see above). There is now exactly one list, + and an entry missing from it fails the test rather than corrupting output. +- ~~**Biggest gap: no executable CI.**~~ `MC/utils/tests/` now provides one that + needs nothing but ROOT and runs in seconds. Still open: wiring it into the + ALICE CI job list — `.github/workflows/` has no ROOT runner, so it needs a + line wherever `test/run_*_tests.sh` are invoked. **Confirm with the O2DPG CI + owners rather than guessing.** + +--- + +## Testing + +```bash +${O2DPG_ROOT}/MC/utils/tests/run_aodbcrewriter_tests.sh +``` + +Needs only ROOT — no simulation, no O2Physics, no GRID, no committed binary +fixture. `makeTestAOD.C` builds a few-kB synthetic AO2D carrying every pathology +the tool exists to repair (non-monotonic + duplicate BCs, duplicate MC +collisions, a split `-1` track group from two sub-timeframe blocks, fwd↔MFT and +fwd→fwd matches, V0/cascade links, `fIndexSliceBCs`, mcparticle mother/daughter +links, and a link-free table for the fast-clone path). + +Three independent layers, in increasing specificity: + +| layer | what it sees | +|---|---| +| `AODBcRewriterValidate(out)` | structural invariants of the output alone | +| `AODBcRewriterCheckLinks(in, out)` | input vs output: no row changed what it points at | +| `testAODBcRewriter(in, out)` | named, physics-readable assertions | + +**The middle layer is the one that matters for this bug class.** Everything in +`AODBcRewriterValidate` is structural, and O2-7098 was structurally perfect — +the row numbers were simply the wrong ones, and in range. Catching that requires +comparing against the input. `AODBcRewriterCheckLinks` does it without +permutation dumps or synthetic tagging: it fingerprints each row from its +non-`fIndex*` branches and compares, per table, the multiset of + +``` +( payload fingerprint of the row, payload fingerprint of each row it points at ) +``` + +before and after. Dedup is handled by canonicalising the *input* side — a +referenced row that did not survive becomes the null link, which is what the +rewriter does — so a tuple present in the output but in no input row is always a +bug. The BC table is fingerprinted on `fGlobalBC` alone, because Stage 0 +*redirects* references onto the surviving row rather than nulling them. + +Because it needs no synthetic data, run it on real production files too: + +```cpp +root -l -b -q -e '.L AODBcRewriter.C' \ + -e 'AODBcRewriterCheckLinks("AO2D_pre.root","AO2D.root")' +``` + +Regression-locked: against the pre-fix `master` rewriter the suite reports +`O2fwdtrack: links not preserved` and `MFT-MCH match: sameParticle=1/6`. --- @@ -152,35 +247,44 @@ order strictly follow its paste-join parent. | Section | Function(s) | Purpose | |---------|-------------|---------| -| 1 | `PermMap`, `isBCTable`, `bcIndexBranch`, `mcCollIndexBranch`, `collIndexBranch`, `kPasteJoins`, `isPasteJoinChild` | Core types, name-probe helpers, and the authoritative paste-join list | +| 1 | `PermMap`, `isBCTable`, `bcIndexBranch`, `mcCollIndexBranch`, `collIndexBranch`, `kPasteJoins`, `isPasteJoinChild`, **`kIndexRefs`**, **`isTableNamed`**, **`unregisteredIndexBranches`** | Core types, name-probe helpers, the paste-join list, and the authoritative index-reference registry | | 2 | `ScalarTag`, `tagOf`, `byteSize`, `readAsInt`, `writeAsInt`, `BranchDesc`, `describeBranches` | Generic ROOT branch I/O over raw byte buffers | -| 3 | `rewriteTable` | **Central engine**: writes any table in a given row order, remapping one nominated index column via a PermMap | +| 3 | **`writeTable`**, `permFromRowOrder`, `remapBuffer`, `findPermFor`, **`buildRemaps`** | **Central engine**: writes any table in a given row order, remapping *every* index column it carries | +| 3b | `TablePlan` | Deferred write plan — why planning and writing are separate phases | | 4 | `BCStage0Result`, `stage0_sortBCs` | Sort + deduplicate the BC table; produce `bcPerm` | -| 5 | `stage0_copyBCFlags` | Copy BC flags table following BC row selection | -| 6 | `MCCollKey`, `MCCollKeyHash`, `stage1_BCindexedTables` | Process all BC-indexed tables; deduplicate MCCollisions | -| 7 | `stage2_MCCollIndexedTables` | Process all MCCollision-indexed tables; drop rows whose parent was deduped | +| 5 | `stage0_copyBCFlags` | Plan the BC flags table following BC row selection | +| 6 | `MCCollKey`, `MCCollKeyHash`, `stage1_BCindexedTables` | Plan all BC-indexed tables; deduplicate MCCollisions | +| 7 | `stage2_MCCollIndexedTables` | Plan all MCCollision-indexed tables; drop rows whose parent was deduped | | 9b | `isCollGroupedTrackTable`, `stage1b_reorderTrackTables` | **Stage 1b**: regroup collision-grouped track tables (`O2track_iu`, `O2mfttrack`, `O2fwdtrack`) by remapped `fIndexCollisions` (`-1` sinks to a contiguous tail); publish track perms so children/references follow. Restores the O2 slicing invariant after the BC→collision reorder cascade | -| 8 | `rowOrderFromPerm`, `findPermByPrefix`, `processPasteJoinTables` | Reorder paste-joined tables to follow their parent (1:1 row count guaranteed); remap any of their own index columns value-wise (incl. `fIndexTracks*` via the Stage 1b track perms); copy unrelated tables verbatim | +| 8 | `planRemainingTables` | Paste-joined tables follow their parent's row order; everything else keeps its own | | 9 | `copyNonTreeObjects` | Copy TMap metadata and other non-TTree objects | -| 10 | `processDF` | Orchestrates all stages for one `DF_*` directory | -| 11 | `AODBcRewriter` | Top-level entry: opens files, iterates `DF_*` dirs, preserves compression | +| 10 | `processDF` | Orchestrates the **plan phase** then the **write phase** for one `DF_*` directory | +| 11 | `AODBcRewriterValidate` and helpers | Structural validation of an output file | +| 11b | `payloadFingerprints`, `linkTupleCounts`, **`AODBcRewriterCheckLinks`** | Input-vs-output link preservation — the check that sees the O2-7098 bug class | +| 12 | `AODBcRewriter` | Top-level entry: opens files, iterates `DF_*` dirs, preserves compression | -### `rewriteTable` — the central engine +### `writeTable` — the central engine ```cpp -PermMap rewriteTable(TTree *src, TDirectory *dirOut, - const vector &rowOrder, - const string &indexBranch, - const PermMap &parentPerm); +void writeTable(TTree *src, TDirectory *dirOut, + const vector &rowOrder, + const vector &remaps); ``` - `rowOrder`: which source rows to emit and in what sequence (may be a subset for deduplication, or reordered for sorting) -- `indexBranch`: name of the one index column to remap (e.g. `"fIndexBCs"`), - or `""` for none -- `parentPerm`: the PermMap from the parent stage used to translate the old - index value to a new one -- Returns `srcToOut` PermMap: `srcToOut[srcRow] = outRow`, -1 if dropped +- `remaps`: **every** index column to remap, each with its own PermMap — + obtained from `buildRemaps(src, allPerms)` and therefore from `kIndexRefs` + +There is deliberately **no privileged "primary index"** parameter any more. The +old signature took one nominated index column plus an optional list of "extras", +which meant every stage had to remember to populate the extras. Stage 1b did +not, and that is O2-7098. Do not reintroduce the distinction. + +The row permutation a `rowOrder` implies is `permFromRowOrder(nSrc, rowOrder)` +(`perm[srcRow] = outRow`, -1 if dropped). Stages publish it into `allPerms` +*before* anything is written, which is what lets a table's remaps refer to +tables written later — or to itself. The function handles both scalar branches and VLA (variable-length array) branches generically. For VLAs it pre-scans the count branch to find the @@ -188,6 +292,21 @@ maximum array length and allocates buffers accordingly. Input and output branches share the same raw byte buffers; ROOT handles the VLA count implicitly through the shared count buffer. +### The two phases in `processDF` + +``` +PLAN stage0 -> stage1 -> stage2 -> stage1b -> planRemainingTables + each appends a TablePlan and publishes its PermMap into allPerms +WRITE for every plan: buildRemaps(src, allPerms) then writeTable(...) + (identity row order and no remaps -> fast CloneTree instead) +``` + +Note `allPerms` holds **reference-remapping** semantics, not plain row +permutations: for the BC table it is `bcPerm`, where several old rows collapse +onto the surviving one, because that is what references into a deduplicated BC +table need. For `O2mccollision` a dropped duplicate maps to -1 instead — that is +the existing, deliberate behaviour (see resolved gap 7), not an oversight. + --- ## MCCollision deduplication @@ -212,18 +331,41 @@ row whose `fIndexMcCollisions` pointed to a dropped row is also dropped. These were identified during the refactor but not yet implemented: -### 1. `fIndexCollisions` inside `O2mccollision` is not remapped +### 1. Tables SORTED BY a reference into a reordered table are not re-sorted -`O2mccollision` has both `fIndexBCs` (handled) and `fIndexCollisions` (linking -back to the reconstructed `O2collision` row). After Stage 1 reorders -`O2collision`, this second index in `O2mccollision` becomes stale. +**This is the remaining member of the O2-7098 family — open, and the natural +follow-up PR.** Stage 1b reorders `O2track_iu` / `O2mfttrack` / `O2fwdtrack`. +Several other tables are *stored sorted by* a reference into one of those (or +into `O2collision`). Their values are now correctly remapped, but their rows are +left where they were, so the ordering is destroyed: -**Fix**: After `stage1_BCindexedTables` runs, find `collPerm` (the PermMap for -`O2collision_*`) in `stage1Perms`, then apply a second `rewriteTable` pass on -`O2mccollision_*` to remap `fIndexCollisions` via `collPerm`. The -`ExtraRemap` mechanism in `rewriteTable` already supports this pattern. +| table | key | sorted in input? | +|---|---|---| +| `O2fwdtrkcl` | `fIndexFwdTracks` | yes | +| `O2ambiguoustrack` | `fIndexTracks` | yes | +| `O2trackqa_003` | `fIndexTracks` | yes | +| `O2v0_002`, `O2cascade_001`, `O2decay3body` | `fIndexCollisions` | yes | +| `O2mfttrackcov` | `fIndexMFTTracks` | **no** — so not an invariant there | -### 2. Deduplication key could be strengthened +(measured on `example_AOD/AO2D_pre.root`.) This is the same failure mode as the +split `-1` group that produced the `ArrowTableSlicingCache::validateOrder` +FATAL — not yet observed in the wild, but structurally identical. + +**Suggested fix** — data-derived rather than another hardcoded list: *if `T.B` +was sorted in the input and `B`'s referent was reordered, re-sort `T` by the +remapped `B`.* That rule reproduces Stage 1b's behaviour and correctly leaves +`O2mfttrackcov` alone. It is deliberately **not** in the O2-7098 fix: it changes +the row order of tables that are currently untouched, so it deserves its own +review. `AODBcRewriterCheckLinks` is already in place to police it. + +### ~~2. `fIndexCollisions` inside `O2mccollision` is not remapped~~ (MOOT) + +Checked against three real AO2Ds (`example_AOD/`, `bigger/`, `bigger2/`): +`O2mccollision_001` carries **only** `fIndexBCs` in the current schema, so there +is nothing to remap. Should the column reappear, `kIndexRefs` now handles it +with no code change. + +### 3. Deduplication key could be strengthened The current `(newBCrow, fEventWeight)` key is a good heuristic. A more robust key would additionally include `fImpactParameter` and/or `fGeneratorsID` if @@ -231,20 +373,21 @@ those branches are present. Consider making the key construction a small helper function that probes which fields are available and builds the strongest possible key. -### 3. `O2mccollision` has two potential parents for paste-join lookup +### ~~4. `O2mccollision` has two potential parents for paste-join lookup~~ (PARTLY RESOLVED) -In `processDF`, the MCColl PermMap is extracted by scanning `stage1Perms` for -a name beginning with `"O2mccollision"`. If the DF contains both -`O2mccollision_000` and `O2mccollision_001` (schema version coexistence), -only the first found is used. Add a warning and handle this explicitly if it -becomes relevant. +Referent resolution now goes through `findPermFor` / `isTableNamed`, which +matches `P` or `P_` and, when several schema versions coexist, picks the +lexicographically smallest name **deterministically** instead of whatever the +`unordered_map` happened to yield first. Still no warning for the ambiguous +case — add one if schema-version coexistence ever becomes real. -### 4. Paste-join size-mismatch fallback is silent-ish +### 5. Paste-join size-mismatch fallback is silent-ish When a paste-joined table has a different row count from its parent (schema -drift), the tool falls back to `CloneTree(-1, "fast")` and prints a warning. -This produces a structurally inconsistent output. Consider making this a hard -error, or implement a best-effort row-count reconciliation. +drift), the tool now keeps the child's own row order — index remaps still get +applied, so nothing is left dangling — and prints a `[warn]`. The output is +still structurally inconsistent and the validator's paste-join parity check will +`[FAIL]` on it. Consider making it a hard error, or reconciling row counts. ### ~~5. No validation pass~~ (RESOLVED) @@ -258,11 +401,14 @@ to confirm output correctness. This was the root cause of the O2Physics FATAL `MC particle N has daughter with index M > MC particle table size`. After Stage 2 reorders `O2mcparticle`, the intra-table mother/daughter indices -now get remapped via `ExtraRemap` in the same pass (Section 7). +get remapped through the table's own permutation. `fIndexMcParticles` in label tables (`O2mctracklabel`, `O2mcfwdtracklabel`, -`O2mcmfttracklabel`, `O2mccalolabel`) is also now remapped via the MC-particle -permutation in `processPasteJoinTables` (Section 8). +`O2mcmfttracklabel`, `O2mccalolabel`) is remapped via the MC-particle +permutation. + +*(Since O2-7098 both are plain `kIndexRefs` entries and need no special +casing — `buildRemaps` picks them up like any other column.)* ### ~~8. fIndexSliceBCs in O2ambiguous* not remapped after BC dedup~~ (RESOLVED) @@ -273,12 +419,12 @@ table. It appears in `O2ambiguoustrack`, `O2ambiguousmfttr`, were processed by Stage 1. After BC dedup the slice endpoints would then point past the compacted table. -**Fix**: `processPasteJoinTables` now also accepts the BC permutation -(passed explicitly from `processDF`) and applies it value-wise to any -`fIndexSliceBCs` / `fIndexBCs` / `fIndexBC` column it finds. Validated -against `example_AOD/AO2D_pre.root`: pre-fix the rewritten output had 7 -and 19 out-of-range slice endpoints in DF_3594457012003; post-fix the -validator reports zero. +**Fix**: `bcPerm` is published into `allPerms` under the BC tree's name, so +`buildRemaps` applies it to any `fIndexSliceBCs` / `fIndexBCs` / `fIndexBC` +column it finds. Validated against `example_AOD/AO2D_pre.root`: pre-fix the +rewritten output had 7 and 19 out-of-range slice endpoints in +DF_3594457012003; post-fix the validator reports zero. `testAODBcRewriter` +also asserts the endpoints still name the same bunch crossings. ### ~~7. Paste-join row-count drift on MC-collision dedup~~ (RESOLVED) @@ -291,7 +437,7 @@ to downstream "O2collision_001 is one larger than O2mccollisionlabel" crashes. **Fix**: `kPasteJoins` was extended to cover every joined pair from `AnalysisDataModel.h`. Paste-join children are now *deferred* from Stage 2 -to `processPasteJoinTables`, where they take the parent's row order and have +to `planRemainingTables`, where they take the parent's row order and have their own index columns remapped value-wise. Rows that lose their MC label on dedup now correctly produce `fIndexMcCollisions == -1`, and the row count matches the parent collision table. @@ -309,28 +455,48 @@ group — including the `-1` ambiguous group — stays one contiguous run, as O2 `ArrowTableSlicingCache::validateOrder` requires. **Fix:** Stage 1b (`stage1b_reorderTrackTables`, Section 9b) stable-sorts each -track table by remapped `fIndexCollisions` (`-1` to a contiguous tail), publishes -the track perm, and `processPasteJoinTables` follows it for paste-join children -and remaps every `fIndexTracks*` reference. New validator check +track table by remapped `fIndexCollisions` (`-1` to a contiguous tail) and +publishes the track perm; paste-join children follow it and every +`fIndexTracks*` reference is remapped through it. Validator check `checkCollisionGroupContiguity` flags split groups as `[FAIL] ... fIndexCollisions has N group(s) split into non-contiguous runs`. -**Status:** committed on `fix/aodbcrewriter-track-regroup` (`8bb9d30b3c`), parses -in cling, **not yet run on a real merged AO2D or the failing analysis task.** +**Status:** on `master` since `5597f516`, now run on real merged AO2Ds. Note +this fix is what *introduced* gap 10 below. + +### ~~10. O2fwdtrack match indices reordered but not remapped (O2-7098)~~ (RESOLVED) + +See the **Current work state** section at the top. Stage 1b wrote each table as +soon as it had planned it, so `O2fwdtrack`'s `fIndexMFTTracks` (pointing at a +table reordered later in the same loop) and `fIndexFwdTracks_MatchMCHTrack` +(pointing at itself) kept pre-reorder row numbers. + +**Fix:** planning and writing are now separate phases, and every index column is +derived from the single `kIndexRefs` registry by `buildRemaps` instead of being +enumerated per stage. --- ## Testing checklist -When testing a new AO2D: +Automated: `${O2DPG_ROOT}/MC/utils/tests/run_aodbcrewriter_tests.sh` (see the +**Testing** section above). Run it for any change to this tool. + +When testing a real AO2D by hand: 1. Run `AODBcRewriterValidate("AO2D_rewritten.root")` (Section 11). It checks BC monotonicity, MC-particle intra-table integrity, paste-join - row-count parity for every pair in `kPasteJoins`, and `fIndex*` value - ranges against the referent table. Failures appear as `[FAIL] ...` lines. -2. Check stdout from the rewrite run itself for any `[warn]` lines — these - indicate branches or tables that fell through to a fallback path. -3. If deduplication ran, verify the dropped count is as expected by comparing + row-count parity for every pair in `kPasteJoins`, `fIndex*` value + ranges against the referent table, and that no `fIndex*` column is missing + from `kIndexRefs`. Failures appear as `[FAIL] ...` lines. +2. Run `AODBcRewriterCheckLinks("AO2D_pre.root","AO2D.root")` (Section 11b). + **Do not skip this one** — it is the only check that compares against the + input, and therefore the only one that can see a mis-remapped index. + Roughly 30 s for a 3-DF file with 6M MC particles. +3. Check stdout from the rewrite run itself for any `[warn]` lines — these + indicate branches or tables that fell through to a fallback path, or an + index column the registry does not know about. +4. If deduplication ran, verify the dropped count is as expected by comparing the input DF MCCollision count vs. output. A standalone minimal validation script (kept here for reference; in practice diff --git a/MC/utils/tests/makeTestAOD.C b/MC/utils/tests/makeTestAOD.C new file mode 100644 index 000000000..a437328f8 --- /dev/null +++ b/MC/utils/tests/makeTestAOD.C @@ -0,0 +1,336 @@ +// makeTestAOD.C +// +// Builds a tiny synthetic AO2D that reproduces, in a few kB, every pathology +// AODBcRewriter.C exists to repair — so the tool can be tested in seconds +// without a simulation, a GRID job or a committed binary fixture. +// +// root -l -b -q 'makeTestAOD.C("AO2D_test.root")' +// +// ----------------------------------------------------------------------------- +// WHAT THE FIXTURE CONTAINS +// ----------------------------------------------------------------------------- +// One DF holding two "sub-timeframe" blocks, as o2-aod-merger produces when MC +// timeframes are merged into the parent data file's DF (data-embedding +// anchoring). That layout is what makes all of the following true at once: +// +// * fGlobalBC is non-monotonic — block B's BCs are earlier than block A's +// * fGlobalBC has a duplicate — BC 201 appears in both blocks +// * MCCollisions have a duplicate — same (BC, fEventWeight) in both blocks +// * the tracks' "-1" ambiguous group is SPLIT into two runs, one per block +// +// so the BC sort cascades into a collision reorder, which cascades into a track +// regroup. Every table then has to follow, and every index column has to be +// remapped — which is the whole job. +// +// Row payloads are unique by construction (fX = 10+i for tracks, 1000+i for MFT +// tracks, 2000+i for fwd tracks, ...) so a test can state ground truth as +// "fwd track 2000+i must still point at MFT track 1000+i" and check it after +// the rows have moved. +// +// Cross-table links present here, and why each one matters: +// +// O2fwdtrack.fIndexMFTTracks -> another Stage-1b table +// O2fwdtrack.fIndexFwdTracks_MatchMCHTrack -> ITSELF +// These two are O2-7098. Both referents are reordered in the same stage +// as the referrer, so they only work if writing is deferred until every +// permutation is known. +// O2v0/O2cascade fIndexTracks_Pos/_Neg/fIndexTracks, fIndexV0s +// references into reordered tables from tables that are not reordered. +// O2ambiguoustrack.fIndexSliceBCs -> a [2] slice into a DEDUPLICATED +// table; endpoints must follow the compaction. +// O2mcparticle fIndexArray_Mothers (VLA) / fIndexSlice_Daughters ([2]) +// intra-table links that must follow the table's own reordering. +// paste-join children (O2trackextra_002, O2fwdtrackcov, O2mc*label) +// no index column of their own for the row order — they must follow +// their parent row-for-row. +// O2origin +// no links at all — exercises the fast-clone path. + +#ifndef __CLING__ +#include "TFile.h" +#include "TTree.h" +#include +#include +#include +#endif + +namespace +{ + +// --------------------------------------------------------------------------- +// The fixture layout, spelled out once so tests can refer to it. +// +// BCs (input row -> fGlobalBC): block A: 0:200 1:201 2:202 +// block B: 3:100 4:101 5:201 (5 duplicates 1) +// -> after Stage 0: [100,101,200,201,202], bcPerm = [2,3,4,0,1,3] +// +// Collisions: 0->bc0 1->bc2 | 2->bc3 3->bc4 +// -> remapped to 2,4,0,1 -> reordered to [2,3,0,1] +// +// MCCollisions: 0->bc0 w1.5 1->bc1 w2.5 | 2->bc3 w3.5 3->bc4 w4.5 +// 4->bc5 w2.5 <- same (newBC,weight) as 1: dropped as duplicate +// +// Tracks: A: 0,1 -> coll0 2 -> coll1 3 -> -1 +// B: 4 -> coll2 5 -> coll3 6 -> -1 (-1 group is split) +// MFT tracks: A: 0 -> coll0 1 -> coll1 2 -> -1 +// B: 3 -> coll2 4 -> coll3 5 -> -1 +// Fwd tracks: A: 0 -> coll0 1 -> coll1 2 -> -1 +// B: 3 -> coll2 4 -> coll3 5 -> -1 +// fwd i matches MFT i (all six). fwd 1 -> fwd 0 and fwd 4 -> fwd 3 as +// MCH matches; the rest have none. +// --------------------------------------------------------------------------- + +const int kNBC = 6; +const uint64_t kGlobalBC[] = {200, 201, 202, 100, 101, 201}; + +const int kNColl = 4; +const int kCollBC[] = {0, 2, 3, 4}; + +const int kNMcColl = 5; +const int kMcCollBC[] = {0, 1, 3, 4, 5}; +const float kMcCollWeight[]= {1.5f, 2.5f, 3.5f, 4.5f, 2.5f}; + +const int kNPartPerMcColl = 3; // mother + two daughters +const int kNPart = kNMcColl * kNPartPerMcColl; + +const int kNTrack = 7; +const int kTrackColl[] = {0, 0, 1, -1, 2, 3, -1}; +const int kTrackMcPart[] = {0, 1, 3, -1, 6, 9, -1}; + +const int kNMft = 6; +const int kMftColl[] = {0, 1, -1, 2, 3, -1}; +const int kMftMcPart[] = {0, 3, -1, 6, 9, -1}; + +const int kNFwd = 6; +const int kFwdColl[] = {0, 1, -1, 2, 3, -1}; +const int kFwdMft[] = {0, 1, 2, 3, 4, 5}; // fwd i <-> MFT i +const int kFwdMatch[] = {-1, 0, -1, -1, 3, -1};// fwd -> fwd self-reference +// The MC particle of a fwd track is the one of its matched MFT track: a correct +// rewrite keeps "same particle" at 100%, a broken one collapses it to ~0 — +// exactly the metric O2-7098 was reported with. +const int kFwdMcPart[] = {0, 3, -1, 6, 9, -1}; + +const int kNV0 = 2; +const int kV0Coll[] = {0, 2}; +const int kV0Pos[] = {0, 4}; +const int kV0Neg[] = {1, 5}; + +const int kNCasc = 1; +const int kCascColl[] = {0}; +const int kCascV0[] = {0}; +const int kCascBach[] = {2}; + +const int kNAmb = 2; +const int kAmbTrack[] = {3, 6}; +const int kAmbBCFirst[] = {0, 3}; +const int kAmbBCLast[] = {2, 4}; + +} // namespace + +void makeTestAOD(const char *outFileName = "AO2D_test.root") +{ + TFile f(outFileName, "RECREATE"); + TDirectory *df = f.mkdir("DF_1000000000001"); + df->cd(); + + // ---- O2bc_001 ----------------------------------------------------------- + { + TTree t("O2bc_001", "bcs"); + uint64_t globalBC; int runNumber; uint64_t triggerMask; + t.Branch("fRunNumber", &runNumber, "fRunNumber/I"); + t.Branch("fGlobalBC", &globalBC, "fGlobalBC/l"); + t.Branch("fTriggerMask", &triggerMask, "fTriggerMask/l"); + for (int i = 0; i < kNBC; ++i) { + runNumber = 300000; globalBC = kGlobalBC[i]; triggerMask = 1000 + i; + t.Fill(); + } + t.Write(); + } + + // ---- O2bcflag (paste-joined to O2bc) ------------------------------------ + { + TTree t("O2bcflag", "bc flags"); + uint8_t flags; + t.Branch("fBCFlags", &flags, "fBCFlags/b"); + for (int i = 0; i < kNBC; ++i) { flags = (uint8_t)(i + 1); t.Fill(); } + t.Write(); + } + + // ---- O2collision_001 ---------------------------------------------------- + { + TTree t("O2collision_001", "collisions"); + int bc; float posX; + t.Branch("fIndexBCs", &bc, "fIndexBCs/I"); + t.Branch("fPosX", &posX, "fPosX/F"); + for (int i = 0; i < kNColl; ++i) { bc = kCollBC[i]; posX = 500.f + i; t.Fill(); } + t.Write(); + } + + // ---- O2mccollision_001 -------------------------------------------------- + { + TTree t("O2mccollision_001", "mc collisions"); + int bc; float weight, posX; + t.Branch("fIndexBCs", &bc, "fIndexBCs/I"); + t.Branch("fPosX", &posX, "fPosX/F"); + t.Branch("fEventWeight",&weight, "fEventWeight/F"); + for (int i = 0; i < kNMcColl; ++i) { + bc = kMcCollBC[i]; weight = kMcCollWeight[i]; + // The duplicate must be byte-identical to the row it duplicates, as it + // would be in a real merged file. + posX = (i == 4) ? 600.f + 1 : 600.f + i; + t.Fill(); + } + t.Write(); + } + + // ---- O2mcparticle_001 --------------------------------------------------- + { + TTree t("O2mcparticle_001", "mc particles"); + int mcColl, pdg, motherSize, mothers[4], daughters[2]; + t.Branch("fIndexMcCollisions", &mcColl, "fIndexMcCollisions/I"); + t.Branch("fPdgCode", &pdg, "fPdgCode/I"); + t.Branch("fIndexArray_Mothers_size",&motherSize, "fIndexArray_Mothers_size/I"); + t.Branch("fIndexArray_Mothers", mothers, "fIndexArray_Mothers[fIndexArray_Mothers_size]/I"); + t.Branch("fIndexSlice_Daughters", daughters, "fIndexSlice_Daughters[2]/I"); + for (int c = 0; c < kNMcColl; ++c) { + int base = c * kNPartPerMcColl; + for (int j = 0; j < kNPartPerMcColl; ++j) { + mcColl = c; + pdg = 100 + base + j; // unique per particle + if (j == 0) { // the mother: daughters are the next two rows + motherSize = 0; + daughters[0] = base + 1; + daughters[1] = base + 2; + } else { // a daughter: its mother is the first row + motherSize = 1; + mothers[0] = base; + daughters[0] = -1; + daughters[1] = -1; + } + t.Fill(); + } + } + t.Write(); + } + + // ---- O2track_iu + paste-join children ----------------------------------- + { + TTree t("O2track_iu", "tracks"); + int coll; float x; + t.Branch("fIndexCollisions", &coll, "fIndexCollisions/I"); + t.Branch("fX", &x, "fX/F"); + for (int i = 0; i < kNTrack; ++i) { coll = kTrackColl[i]; x = 10.f + i; t.Fill(); } + t.Write(); + } + { + TTree t("O2trackextra_002", "track extras"); + uint8_t nClsFindable; + t.Branch("fTPCNClsFindable", &nClsFindable, "fTPCNClsFindable/b"); + for (int i = 0; i < kNTrack; ++i) { nClsFindable = (uint8_t)(50 + i); t.Fill(); } + t.Write(); + } + { + TTree t("O2mctracklabel", "track mc labels"); + int mcPart; uint16_t mask; + t.Branch("fIndexMcParticles", &mcPart, "fIndexMcParticles/I"); + t.Branch("fMcMask", &mask, "fMcMask/s"); + for (int i = 0; i < kNTrack; ++i) { mcPart = kTrackMcPart[i]; mask = (uint16_t)i; t.Fill(); } + t.Write(); + } + + // ---- O2mfttrack_001 + label --------------------------------------------- + { + TTree t("O2mfttrack_001", "mft tracks"); + int coll; float x; + t.Branch("fIndexCollisions", &coll, "fIndexCollisions/I"); + t.Branch("fX", &x, "fX/F"); + for (int i = 0; i < kNMft; ++i) { coll = kMftColl[i]; x = 1000.f + i; t.Fill(); } + t.Write(); + } + { + TTree t("O2mcmfttracklabel", "mft mc labels"); + int mcPart; uint16_t mask; + t.Branch("fIndexMcParticles", &mcPart, "fIndexMcParticles/I"); + t.Branch("fMcMask", &mask, "fMcMask/s"); + for (int i = 0; i < kNMft; ++i) { mcPart = kMftMcPart[i]; mask = (uint16_t)(100 + i); t.Fill(); } + t.Write(); + } + + // ---- O2fwdtrack + cov + label ------------------------------------------- + // The table O2-7098 is about: two index columns pointing into tables that are + // reordered in the same stage as this one, one of them itself. + { + TTree t("O2fwdtrack", "fwd tracks"); + int coll, mft, match; float x; + t.Branch("fIndexCollisions", &coll, "fIndexCollisions/I"); + t.Branch("fX", &x, "fX/F"); + t.Branch("fIndexMFTTracks", &mft, "fIndexMFTTracks/I"); + t.Branch("fIndexFwdTracks_MatchMCHTrack", &match, "fIndexFwdTracks_MatchMCHTrack/I"); + for (int i = 0; i < kNFwd; ++i) { + coll = kFwdColl[i]; mft = kFwdMft[i]; match = kFwdMatch[i]; x = 2000.f + i; + t.Fill(); + } + t.Write(); + } + { + TTree t("O2fwdtrackcov", "fwd track cov"); + float sigmaX; + t.Branch("fSigmaX", &sigmaX, "fSigmaX/F"); + for (int i = 0; i < kNFwd; ++i) { sigmaX = 0.5f + i; t.Fill(); } + t.Write(); + } + { + TTree t("O2mcfwdtracklabel", "fwd mc labels"); + int mcPart; uint16_t mask; + t.Branch("fIndexMcParticles", &mcPart, "fIndexMcParticles/I"); + t.Branch("fMcMask", &mask, "fMcMask/s"); + for (int i = 0; i < kNFwd; ++i) { mcPart = kFwdMcPart[i]; mask = (uint16_t)(200 + i); t.Fill(); } + t.Write(); + } + + // ---- O2v0_002 / O2cascade_001 ------------------------------------------- + { + TTree t("O2v0_002", "v0s"); + int coll, pos, neg; + t.Branch("fIndexCollisions", &coll, "fIndexCollisions/I"); + t.Branch("fIndexTracks_Pos", &pos, "fIndexTracks_Pos/I"); + t.Branch("fIndexTracks_Neg", &neg, "fIndexTracks_Neg/I"); + for (int i = 0; i < kNV0; ++i) { coll = kV0Coll[i]; pos = kV0Pos[i]; neg = kV0Neg[i]; t.Fill(); } + t.Write(); + } + { + TTree t("O2cascade_001", "cascades"); + int coll, v0, bach; + t.Branch("fIndexCollisions", &coll, "fIndexCollisions/I"); + t.Branch("fIndexV0s", &v0, "fIndexV0s/I"); + t.Branch("fIndexTracks", &bach, "fIndexTracks/I"); + for (int i = 0; i < kNCasc; ++i) { coll = kCascColl[i]; v0 = kCascV0[i]; bach = kCascBach[i]; t.Fill(); } + t.Write(); + } + + // ---- O2ambiguoustrack (slice into the deduplicated BC table) ------------ + { + TTree t("O2ambiguoustrack", "ambiguous tracks"); + int track, sliceBCs[2]; + t.Branch("fIndexTracks", &track, "fIndexTracks/I"); + t.Branch("fIndexSliceBCs", sliceBCs,"fIndexSliceBCs[2]/I"); + for (int i = 0; i < kNAmb; ++i) { + track = kAmbTrack[i]; sliceBCs[0] = kAmbBCFirst[i]; sliceBCs[1] = kAmbBCLast[i]; + t.Fill(); + } + t.Write(); + } + + // ---- O2origin (no links at all — exercises the fast-clone path) --------- + { + TTree t("O2origin", "origins"); + int dataframeID; + t.Branch("fDataframeID", &dataframeID, "fDataframeID/I"); + dataframeID = 1; t.Fill(); + t.Write(); + } + + f.Write(); + f.Close(); + std::cout << "makeTestAOD: wrote " << outFileName << "\n"; +} diff --git a/MC/utils/tests/run_aodbcrewriter_tests.sh b/MC/utils/tests/run_aodbcrewriter_tests.sh new file mode 100755 index 000000000..f8bda9432 --- /dev/null +++ b/MC/utils/tests/run_aodbcrewriter_tests.sh @@ -0,0 +1,105 @@ +#!/bin/bash +# +# Self-contained regression test for MC/utils/AODBcRewriter.C. +# +# ${O2DPG_ROOT}/MC/utils/tests/run_aodbcrewriter_tests.sh +# +# Needs nothing but ROOT — no simulation, no O2Physics, no GRID, no committed +# binary fixture. Runs in a couple of seconds. +# +# It builds a synthetic AO2D carrying every pathology the rewriter exists to +# repair (see makeTestAOD.C), rewrites it, and then applies three independent +# layers of checking: +# +# 1. AODBcRewriterValidate() — structural invariants of the output alone +# (BC monotonicity, paste-join row parity, +# index ranges, collision-group contiguity, +# and that no fIndex* column is unregistered) +# 2. AODBcRewriterCheckLinks() — input vs output: no row changed what it +# points at. This is the layer that catches +# O2-7098; the structural checks cannot, since +# a mis-remapped index is still perfectly in +# range. +# 3. testAODBcRewriter() — named, physics-readable assertions, headed by +# the MFT/MCH same-particle check the bug was +# reported with. +# +# Exit code 0 = pass. + +set -o pipefail + +MY_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +UTILS_DIR="$(dirname "${MY_DIR}")" +REWRITER="${UTILS_DIR}/AODBcRewriter.C" + +if ! command -v root > /dev/null 2>&1; then + echo "ERROR: no ROOT in PATH — load an O2/O2Physics/ROOT environment first" >&2 + exit 1 +fi +if [ ! -f "${REWRITER}" ]; then + echo "ERROR: cannot find ${REWRITER}" >&2 + exit 1 +fi + +WORKDIR=$(mktemp -d "${TMPDIR:-/tmp}/aodbcrewriter-test-XXXXXX") +trap 'rm -rf "${WORKDIR}"' EXIT +cd "${WORKDIR}" || exit 1 + +IN="AO2D_test.root" +OUT="AO2D_test_rewritten.root" +RC=0 + +run_step() +{ + local name="$1"; shift + echo "=== ${name} ===" + if ! "$@" > "${name}.log" 2>&1; then + echo "--- ${name} FAILED, log follows ---" >&2 + cat "${name}.log" >&2 + RC=1 + return 1 + fi + # ROOT macros signal failure through their return value, which the shell + # does not see; look for the markers as well. + if grep -q '\[FAIL\]' "${name}.log"; then + echo "--- ${name} reported [FAIL] lines ---" >&2 + grep -E '\[FAIL\]|FAILED' "${name}.log" >&2 + RC=1 + return 1 + fi + return 0 +} + +echo "Working in ${WORKDIR}" + +run_step "01-make-fixture" \ + root -l -b -q "${MY_DIR}/makeTestAOD.C(\"${IN}\")" + +run_step "02-rewrite" \ + root -l -b -q "${REWRITER}(\"${IN}\",\"${OUT}\")" + +# From here on every step needs the rewritten file. +if [ ! -f "${OUT}" ]; then + echo "ERROR: rewriter produced no output" >&2 + exit 1 +fi + +run_step "03-validate" \ + root -l -b -q -e ".L ${REWRITER}" -e "if (!AODBcRewriterValidate(\"${OUT}\")) gSystem->Exit(1);" + +run_step "04-check-links" \ + root -l -b -q -e ".L ${REWRITER}" -e "if (!AODBcRewriterCheckLinks(\"${IN}\",\"${OUT}\")) gSystem->Exit(1);" + +run_step "05-assertions" \ + root -l -b -q -e ".L ${REWRITER}" -e ".L ${MY_DIR}/testAODBcRewriter.C" \ + -e "if (testAODBcRewriter(\"${IN}\",\"${OUT}\") != 0) gSystem->Exit(1);" + +# Surface the readable assertion results even on success. +[ -f 05-assertions.log ] && grep -E '\[ ok \]|\[FAIL\]|PASSED|FAILED' 05-assertions.log + +if [ "${RC}" -eq 0 ]; then + echo "AODBcRewriter tests: PASSED" +else + echo "AODBcRewriter tests: FAILED" +fi +exit "${RC}" diff --git a/MC/utils/tests/testAODBcRewriter.C b/MC/utils/tests/testAODBcRewriter.C new file mode 100644 index 000000000..a0357aa82 --- /dev/null +++ b/MC/utils/tests/testAODBcRewriter.C @@ -0,0 +1,294 @@ +// testAODBcRewriter.C +// +// Physics-readable assertions on a rewritten AO2D, on top of the generic +// AODBcRewriterValidate() / AODBcRewriterCheckLinks() checks. +// +// root -l -b -q 'testAODBcRewriter.C("AO2D_test.root","AO2D_test_rewritten.root")' +// +// Returns 0 on success, the number of failed assertions otherwise. +// +// Expectations are derived from the INPUT file rather than hard-coded, so the +// fixture in makeTestAOD.C can grow without this file having to be kept in +// sync. What is hard-coded is only the *invariant*: a rewrite permutes rows, +// it never changes which row points at which. +// +// The headline assertion is the one from the O2-7098 report: for a fwd track +// with an MFT match, the MFT leg and the MCH leg must belong to the same MC +// particle. In this fixture the matching is perfect by construction, so the +// correct answer is 100% and a mis-remapped fIndexMFTTracks gives ~0% — +// the same signature seen in the anchored pO production. + +#ifndef __CLING__ +#include "TFile.h" +#include "TTree.h" +#include +#include +#include +#include +#include +#include +#endif + +namespace +{ + +int gFailures = 0; + +void fail(const std::string &what) +{ + std::cerr << " [FAIL] " << what << "\n"; + ++gFailures; +} + +void pass(const std::string &what) +{ + std::cout << " [ ok ] " << what << "\n"; +} + +TTree *get(TDirectory *d, const char *name) +{ + TTree *t = dynamic_cast(d->Get(name)); + if (!t) fail(std::string("table ") + name + " missing"); + return t; +} + +std::vector readInt(TTree *t, const char *branch) +{ + std::vector v; + if (!t || !t->GetBranch(branch)) return v; + Int_t x = 0; + t->ResetBranchAddresses(); + t->SetBranchAddress(branch, &x); + v.reserve(t->GetEntries()); + for (Long64_t i = 0; i < t->GetEntries(); ++i) { t->GetEntry(i); v.push_back(x); } + t->ResetBranchAddresses(); + return v; +} + +std::vector readFloat(TTree *t, const char *branch) +{ + std::vector v; + if (!t || !t->GetBranch(branch)) return v; + Float_t x = 0; + t->ResetBranchAddresses(); + t->SetBranchAddress(branch, &x); + v.reserve(t->GetEntries()); + for (Long64_t i = 0; i < t->GetEntries(); ++i) { t->GetEntry(i); v.push_back(x); } + t->ResetBranchAddresses(); + return v; +} + +std::vector readULong(TTree *t, const char *branch) +{ + std::vector v; + if (!t || !t->GetBranch(branch)) return v; + ULong64_t x = 0; + t->ResetBranchAddresses(); + t->SetBranchAddress(branch, &x); + v.reserve(t->GetEntries()); + for (Long64_t i = 0; i < t->GetEntries(); ++i) { t->GetEntry(i); v.push_back(x); } + t->ResetBranchAddresses(); + return v; +} + +// The multiset of (referrer payload, referent payload) pairs implied by an +// index column. Rows with no link contribute (payload, sentinel). +std::multimap linkPairs(const std::vector &idx, + const std::vector &ownPayload, + const std::vector &refPayload) +{ + std::multimap pairs; + for (size_t i = 0; i < idx.size() && i < ownPayload.size(); ++i) { + float ref = -1.f; + if (idx[i] >= 0 && (size_t)idx[i] < refPayload.size()) ref = refPayload[idx[i]]; + pairs.emplace(ownPayload[i], ref); + } + return pairs; +} + +void expectSameLinks(const char *what, + const std::multimap &in, + const std::multimap &out) +{ + if (in.size() != out.size()) { + fail(std::string(what) + ": row count changed (" + + std::to_string(in.size()) + " -> " + std::to_string(out.size()) + ")"); + return; + } + std::vector> a(in.begin(), in.end()), b(out.begin(), out.end()); + std::sort(a.begin(), a.end()); + std::sort(b.begin(), b.end()); + if (a != b) { + size_t shown = 0; + for (size_t i = 0; i < a.size() && shown < 5; ++i) { + if (a[i] != b[i]) { + std::cerr << " row " << a[i].first << " pointed at " << a[i].second + << " before, at " << b[i].second << " after\n"; + ++shown; + } + } + fail(std::string(what) + ": links not preserved"); + return; + } + pass(std::string(what) + ": links preserved (" + std::to_string(a.size()) + " rows)"); +} + +} // namespace + +int testAODBcRewriter(const char *inFileName = "AO2D_test.root", + const char *outFileName = "AO2D_test_rewritten.root") +{ + gFailures = 0; + std::cout << "testAODBcRewriter: " << inFileName << " -> " << outFileName << "\n"; + + TFile fin(inFileName, "READ"); + TFile fout(outFileName, "READ"); + if (fin.IsZombie()) { std::cerr << "cannot open " << inFileName << "\n"; return 1; } + if (fout.IsZombie()) { std::cerr << "cannot open " << outFileName << "\n"; return 1; } + + const char *dfName = "DF_1000000000001"; + TDirectory *din = dynamic_cast(fin.Get(dfName)); + TDirectory *dout = dynamic_cast(fout.Get(dfName)); + if (!din || !dout) { std::cerr << "DF " << dfName << " missing\n"; return 1; } + + // ---- Stage 0 did its job ------------------------------------------------- + { + auto bc = readULong(get(dout, "O2bc_001"), "fGlobalBC"); + bool mono = true; + for (size_t i = 1; i < bc.size(); ++i) if (bc[i] <= bc[i - 1]) mono = false; + if (!mono) fail("O2bc_001: fGlobalBC not strictly monotonic after rewrite"); + else pass("O2bc_001: fGlobalBC strictly increasing"); + if (bc.size() != 5) fail("O2bc_001: expected 5 unique BCs, got " + std::to_string(bc.size())); + else pass("O2bc_001: duplicate BC removed (6 -> 5)"); + } + + // ---- MCCollision deduplication ------------------------------------------ + { + Long64_t n = get(dout, "O2mccollision_001")->GetEntries(); + if (n != 4) fail("O2mccollision_001: expected 4 rows after dedup, got " + std::to_string(n)); + else pass("O2mccollision_001: duplicate MC collision removed (5 -> 4)"); + } + + // ---- The O2-7098 links --------------------------------------------------- + { + auto fwdXIn = readFloat(get(din, "O2fwdtrack"), "fX"); + auto fwdXOut = readFloat(get(dout, "O2fwdtrack"), "fX"); + auto mftXIn = readFloat(get(din, "O2mfttrack_001"), "fX"); + auto mftXOut = readFloat(get(dout, "O2mfttrack_001"), "fX"); + + expectSameLinks("O2fwdtrack.fIndexMFTTracks", + linkPairs(readInt(get(din, "O2fwdtrack"), "fIndexMFTTracks"), fwdXIn, mftXIn), + linkPairs(readInt(get(dout, "O2fwdtrack"), "fIndexMFTTracks"), fwdXOut, mftXOut)); + + expectSameLinks("O2fwdtrack.fIndexFwdTracks_MatchMCHTrack (self-reference)", + linkPairs(readInt(get(din, "O2fwdtrack"), "fIndexFwdTracks_MatchMCHTrack"), fwdXIn, fwdXIn), + linkPairs(readInt(get(dout, "O2fwdtrack"), "fIndexFwdTracks_MatchMCHTrack"), fwdXOut, fwdXOut)); + } + + // ---- The reported symptom: MFT leg and MCH leg of the same global muon --- + // must carry the same MC particle. This is Maurice Coquet's tableMakerMC + // check, reduced to the label tables. Perfect matching by construction here. + { + auto mftIdx = readInt(get(dout, "O2fwdtrack"), "fIndexMFTTracks"); + auto fwdLbl = readInt(get(dout, "O2mcfwdtracklabel"), "fIndexMcParticles"); + auto mftLbl = readInt(get(dout, "O2mcmfttracklabel"), "fIndexMcParticles"); + int checked = 0, same = 0; + for (size_t i = 0; i < mftIdx.size() && i < fwdLbl.size(); ++i) { + if (mftIdx[i] < 0 || (size_t)mftIdx[i] >= mftLbl.size()) continue; + ++checked; + if (fwdLbl[i] == mftLbl[mftIdx[i]]) ++same; + } + if (checked == 0) fail("MFT-MCH match check: nothing to check"); + else if (same != checked) fail("MFT-MCH match: sameParticle=" + std::to_string(same) + + "/" + std::to_string(checked) + ", expected all (this is O2-7098)"); + else pass("MFT-MCH match: sameParticle=" + std::to_string(same) + + "/" + std::to_string(checked) + " (100%)"); + } + + // ---- References into the reordered track tables from elsewhere ----------- + { + auto trkXIn = readFloat(get(din, "O2track_iu"), "fX"); + auto trkXOut = readFloat(get(dout, "O2track_iu"), "fX"); + + // V0s have no float payload of their own; identify them by their positive + // leg, which is unique in this fixture. + auto v0PosIn = readInt(get(din, "O2v0_002"), "fIndexTracks_Pos"); + auto v0PosOut = readInt(get(dout, "O2v0_002"), "fIndexTracks_Pos"); + auto v0NegIn = readInt(get(din, "O2v0_002"), "fIndexTracks_Neg"); + auto v0NegOut = readInt(get(dout, "O2v0_002"), "fIndexTracks_Neg"); + std::vector v0IdIn, v0IdOut; + for (auto p : v0PosIn) v0IdIn.push_back(p >= 0 && (size_t)p < trkXIn.size() ? trkXIn[p] : -1.f); + for (auto p : v0PosOut) v0IdOut.push_back(p >= 0 && (size_t)p < trkXOut.size() ? trkXOut[p] : -1.f); + expectSameLinks("O2v0_002.fIndexTracks_Neg", + linkPairs(v0NegIn, v0IdIn, trkXIn), + linkPairs(v0NegOut, v0IdOut, trkXOut)); + } + + // ---- Paste-join children still line up with their parent ---------------- + { + auto fwdXOut = readFloat(get(dout, "O2fwdtrack"), "fX"); + auto sigmaIn = readFloat(get(din, "O2fwdtrackcov"), "fSigmaX"); + auto sigmaOut= readFloat(get(dout, "O2fwdtrackcov"), "fSigmaX"); + auto fwdXIn = readFloat(get(din, "O2fwdtrack"), "fX"); + std::map expect; // fX -> fSigmaX, from the input + for (size_t i = 0; i < fwdXIn.size() && i < sigmaIn.size(); ++i) expect[fwdXIn[i]] = sigmaIn[i]; + bool aligned = (fwdXOut.size() == sigmaOut.size()); + for (size_t i = 0; aligned && i < fwdXOut.size(); ++i) + if (expect.count(fwdXOut[i]) == 0 || expect[fwdXOut[i]] != sigmaOut[i]) aligned = false; + if (!aligned) fail("O2fwdtrackcov: paste-join alignment with O2fwdtrack broken"); + else pass("O2fwdtrackcov: follows O2fwdtrack row-for-row"); + } + + // ---- Intra-table MC particle links -------------------------------------- + { + TTree *mcp = get(dout, "O2mcparticle_001"); + auto pdg = readInt(mcp, "fPdgCode"); + Int_t daughters[2] = {-1, -1}; + mcp->ResetBranchAddresses(); + mcp->SetBranchAddress("fIndexSlice_Daughters", daughters); + int checked = 0, bad = 0; + for (Long64_t i = 0; i < mcp->GetEntries(); ++i) { + mcp->GetEntry(i); + if (daughters[0] < 0) continue; + ++checked; + // In the fixture a mother with fPdgCode P has daughters P+1 and P+2. + if (daughters[1] - daughters[0] != 1) { ++bad; continue; } + if (pdg[daughters[0]] != pdg[i] + 1 || pdg[daughters[1]] != pdg[i] + 2) ++bad; + } + mcp->ResetBranchAddresses(); + if (checked == 0) fail("O2mcparticle_001: no daughter slices found"); + else if (bad) fail("O2mcparticle_001: " + std::to_string(bad) + "/" + + std::to_string(checked) + " daughter slices point at the wrong particles"); + else pass("O2mcparticle_001: all " + std::to_string(checked) + + " daughter slices still point at the right particles"); + } + + // ---- Slice into the deduplicated BC table ------------------------------- + { + auto bcIn = readULong(get(din, "O2bc_001"), "fGlobalBC"); + auto bcOut = readULong(get(dout, "O2bc_001"), "fGlobalBC"); + TTree *aIn = get(din, "O2ambiguoustrack"); + TTree *aOut = get(dout, "O2ambiguoustrack"); + Int_t sIn[2] = {-1, -1}, sOut[2] = {-1, -1}; + aIn->ResetBranchAddresses(); aIn->SetBranchAddress("fIndexSliceBCs", sIn); + aOut->ResetBranchAddresses(); aOut->SetBranchAddress("fIndexSliceBCs", sOut); + int bad = 0; + Long64_t n = std::min(aIn->GetEntries(), aOut->GetEntries()); + for (Long64_t i = 0; i < n; ++i) { + aIn->GetEntry(i); aOut->GetEntry(i); + for (int j = 0; j < 2; ++j) { + if (sIn[j] < 0 || (size_t)sIn[j] >= bcIn.size()) continue; + if (sOut[j] < 0 || (size_t)sOut[j] >= bcOut.size()) { ++bad; continue; } + if (bcIn[sIn[j]] != bcOut[sOut[j]]) ++bad; + } + } + aIn->ResetBranchAddresses(); aOut->ResetBranchAddresses(); + if (bad) fail("O2ambiguoustrack.fIndexSliceBCs: " + std::to_string(bad) + + " endpoint(s) no longer point at the same bunch crossing"); + else pass("O2ambiguoustrack.fIndexSliceBCs: endpoints follow the BC compaction"); + } + + if (gFailures == 0) std::cout << "testAODBcRewriter: ALL CHECKS PASSED\n"; + else std::cout << "testAODBcRewriter: " << gFailures << " CHECK(S) FAILED\n"; + return gFailures; +} From b8855f1562aaed0d2f939098bbc14fd598e7a4d3 Mon Sep 17 00:00:00 2001 From: Sandro Wenzel Date: Tue, 28 Jul 2026 23:19:39 +0200 Subject: [PATCH 2/2] AODBcRewriter: re-sort tables stored sorted by a reordered reference Second member of the O2-7098 family. Stage 1b reorders O2track_iu/ O2mfttrack/O2fwdtrack; several other tables (O2fwdtrkcl, O2ambiguoustrack, O2trackqa_003, O2v0_002, O2cascade_001, O2decay3body) are stored SORTED BY a reference into one of those (or into O2collision). Their index values were remapped correctly, but their rows stayed put, so a sorted column came out unsorted -- the same defect as the split "-1" group that once caused [FATAL] Table Tracks_IU index fIndexCollisions has a group with index -1 that is split by 776 Confirmed on real files: O2fwdtrkcl and O2trackqa_003 came out unsorted in every DF of two of three anchored-MC samples tested. Fix: findGroupingColumn() + resortByGroupingColumn(), derived from the data rather than another hardcoded table list -- "if T.B is non-decreasing on input and B's referent moved, re-sort T by the remapped B". Leaves O2mfttrackcov alone (not sorted on input). Iterated to a fixed point, since O2cascade_001 is sorted by fIndexV0s and O2v0_002 may itself have just moved. AODBcRewriterCheckLinks() now also asserts sortedness is preserved. Fixture extended with O2fwdtrkcl and a second cascade to exercise the O2v0->O2cascade dependency; against the previous commit the suite fails 3 assertions and 6 link checks, and passes with this one. Co-Authored-By: Claude Opus 5 --- MC/utils/AODBcRewriter.C | 179 ++++++++++++++++++++++++++++- MC/utils/CLAUDE.md | 42 ++++--- MC/utils/tests/makeTestAOD.C | 28 ++++- MC/utils/tests/testAODBcRewriter.C | 34 ++++++ 4 files changed, 260 insertions(+), 23 deletions(-) diff --git a/MC/utils/AODBcRewriter.C b/MC/utils/AODBcRewriter.C index f03d013a1..4080c9044 100644 --- a/MC/utils/AODBcRewriter.C +++ b/MC/utils/AODBcRewriter.C @@ -1075,15 +1075,155 @@ static void stage1b_reorderTrackTables( } } +// ---------------------------------------------------------------------------- +// Re-sorting tables that are STORED SORTED BY a reference +// +// Stage 1b exists because a track table grouped by fIndexCollisions stops being +// sliceable once the collision table is reordered. Exactly the same is true of +// every other table stored sorted by a reference into a table this tool +// reorders — O2v0_002 and O2cascade_001 by fIndexCollisions, O2fwdtrkcl by +// fIndexFwdTracks, O2ambiguoustrack and O2trackqa_003 by fIndexTracks. +// Remapping their values while leaving their rows in place turns a sorted +// column into an unsorted one, which is the same defect that produced +// "Table ... index fIndexCollisions has a group with index -1 that is split". +// +// Rather than hardcode which tables are grouped by what — the enumeration habit +// that caused O2-7098 — the decision is derived from the data: IF a column is +// non-decreasing in the input, THEN it is an ordering the file carries and the +// output must preserve it. That is self-maintaining across schema changes, and +// it correctly leaves O2mfttrackcov alone: its fIndexMFTTracks is not sorted in +// the input, so there is no ordering to preserve. + +struct ResortCandidate { + size_t planIdx = 0; + std::string tableName; + std::string keyBranch; + std::vector keyValues; // raw input values of keyBranch + std::vector refPrefixes; // referent of keyBranch, from kIndexRefs +}; + +// Read a scalar Int_t-like index column. Returns false for VLA / fixed-array +// columns, which are not row orderings. +static bool readScalarIndexColumn(TTree *t, const char *branch, + std::vector &out) { + TBranch *br = t->GetBranch(branch); + if (!br) return false; + TLeaf *leaf = static_cast(br->GetListOfLeaves()->At(0)); + if (!leaf || leaf->GetLeafCount() || leaf->GetLen() != 1) return false; + ScalarTag tag = tagOf(leaf); + size_t sz = byteSize(tag); + if (sz == 0) return false; + std::vector buf(sz, 0); + br->SetAddress(buf.data()); + out.clear(); + out.reserve(t->GetEntries()); + for (Long64_t i = 0; i < t->GetEntries(); ++i) { + br->GetEntry(i); + out.push_back(readAsInt(buf.data(), tag)); + } + br->ResetAddress(); + return true; +} + +// The convention this tool writes (and Stage 1b establishes): valid values +// ascending, the -1 "ambiguous" group as one contiguous run at the end. +static bool isOrderedWithNullsLast(const std::vector &v) { + Long64_t prev = -1; + bool seenNull = false; + for (auto x : v) { + if (x < 0) { seenNull = true; continue; } + if (seenNull) return false; // a valid value after a null: not this convention + if (x < prev) return false; + prev = x; + } + return true; +} + +// Pick the column a table is stored sorted by, if any. fIndexCollisions wins +// when several qualify, since that is the grouping O2's slicing cache checks. +static bool findGroupingColumn(TTree *src, std::string &keyBranch, + std::vector &keyValues, + std::vector &refPrefixes) { + if (!src || src->GetEntries() < 2) return false; + bool found = false; + for (auto &[branchName, prefixes] : kIndexRefs) { + std::vector vals; + if (!readScalarIndexColumn(src, branchName.c_str(), vals)) continue; + if (!isOrderedWithNullsLast(vals)) continue; + bool preferred = (branchName == "fIndexCollisions"); + if (found && !preferred) continue; + keyBranch = branchName; + keyValues = std::move(vals); + refPrefixes = prefixes; + found = true; + if (preferred) break; + } + return found; +} + +// Re-sort each candidate by its remapped grouping column. Iterated to a fixed +// point because these tables reference each other: O2cascade_001 is sorted by +// fIndexV0s, and O2v0_002 may itself have just been re-sorted. +static void resortByGroupingColumn( + std::vector &plans, + std::unordered_map &allPerms, + const std::vector &candidates) { + + const int kMaxPasses = 8; + int pass = 0; + for (; pass < kMaxPasses; ++pass) { + bool changed = false; + for (auto &cand : candidates) { + const PermMap *refPerm = findPermFor(allPerms, cand.refPrefixes); + if (!refPerm) continue; // referent absent from this DF + + Long64_t n = (Long64_t)cand.keyValues.size(); + struct SortEntry { Long64_t key; Long64_t srcRow; }; + std::vector entries; + entries.reserve(n); + for (Long64_t i = 0; i < n; ++i) { + Long64_t old = cand.keyValues[i]; + Long64_t nw = (old >= 0 && old < (Long64_t)refPerm->size()) ? (*refPerm)[old] : -1; + entries.push_back({nw, i}); + } + // Same ordering convention as Stage 1: -1 sinks to a contiguous tail. + std::stable_sort(entries.begin(), entries.end(), + [](const SortEntry &a, const SortEntry &b) { + if (a.key < 0 && b.key >= 0) return false; + if (a.key >= 0 && b.key < 0) return true; + return a.key < b.key; + }); + std::vector rowOrder; + rowOrder.reserve(n); + for (auto &e : entries) rowOrder.push_back(e.srcRow); + + if (rowOrder == plans[cand.planIdx].rowOrder) continue; + plans[cand.planIdx].rowOrder = rowOrder; + allPerms[cand.tableName] = permFromRowOrder(n, rowOrder); + changed = true; + std::cout << " Re-sort: " << cand.tableName << " by remapped " + << cand.keyBranch << " (was sorted by it on input)\n"; + } + if (!changed) break; + } + if (pass == kMaxPasses) + std::cerr << " [warn] re-sort did not reach a fixed point after " + << kMaxPasses << " passes — check for a reference cycle\n"; +} + // Plan every table not yet claimed by an earlier stage: paste-join children -// follow their parent's row order, everything else keeps its own. The index -// columns they carry are NOT enumerated here any more — buildRemaps() derives -// them from kIndexRefs when processDF writes. +// follow their parent's row order, everything else keeps its own — unless it is +// stored sorted by a reference, in which case resortByGroupingColumn() below +// re-establishes that ordering. The index columns these tables carry are NOT +// enumerated here — buildRemaps() derives them from kIndexRefs when processDF +// writes. static void planRemainingTables( TDirectory *dirIn, std::vector &plans, std::unordered_map &allPerms, std::unordered_set &planned) { + std::vector resortCandidates; + TIter it(dirIn->GetListOfKeys()); while (TKey *key = static_cast(it())) { if (TString(key->GetClassName()) != "TTree") continue; @@ -1139,10 +1279,26 @@ static void planRemainingTables( std::iota(rowOrder.begin(), rowOrder.end(), 0LL); } + // A table that arrives here with its own row order may still be STORED + // SORTED BY one of its index columns; if that column's referent gets + // reordered, the sortedness has to be re-established. Record what is + // needed for that; the decision is made below, once every table in this + // stage has a permutation. + if (!parentPerm) { + ResortCandidate cand; + if (findGroupingColumn(src, cand.keyBranch, cand.keyValues, cand.refPrefixes)) { + cand.planIdx = plans.size(); + cand.tableName = tname; + resortCandidates.push_back(std::move(cand)); + } + } + allPerms[tname] = permFromRowOrder(nSrc, rowOrder); plans.push_back({tname, std::move(rowOrder)}); planned.insert(tname); } + + resortByGroupingColumn(plans, allPerms, resortCandidates); } // ============================================================================ @@ -1826,6 +1982,23 @@ static bool checkLinksDF(TDirectory *din, TDirectory *dout, const char *dfName) linksOut.push_back({branchName, &fpOut[refName], nullptr}); } + // Ordering preservation: a column that is sorted on input describes a + // grouping the file carries, and O2's slicing cache relies on it. Remapping + // the values while leaving the rows in place silently destroys it — the same + // defect as the split "-1" group, just in a different table. + for (auto &[branchName, prefixes] : kIndexRefs) { + std::vector vIn, vOut; + if (!readScalarIndexColumn(tIn, branchName.c_str(), vIn)) continue; + if (!isOrderedWithNullsLast(vIn)) continue; // no ordering to preserve + if (!readScalarIndexColumn(tOut, branchName.c_str(), vOut)) continue; + if (!isOrderedWithNullsLast(vOut)) { + std::cerr << " [FAIL] " << dfName << ": " << tn << "." << branchName + << " was sorted on input but is not on output" + " (grouping destroyed — slicing will misbehave)\n"; + ok = false; + } + } + auto cIn = linkTupleCounts(tIn, fpIn[tn], linksIn); auto cOut = linkTupleCounts(tOut, fpOut[tn], linksOut); diff --git a/MC/utils/CLAUDE.md b/MC/utils/CLAUDE.md index 52bc83901..13d799382 100644 --- a/MC/utils/CLAUDE.md +++ b/MC/utils/CLAUDE.md @@ -49,6 +49,11 @@ practice **every merged MC AO2D since 4 Jun 2026 is affected**. test instead of silently mis-linking data. 5. **`AODBcRewriterCheckLinks()` (Section 11b)** — the check that can actually see this bug class; see "Testing" below. +6. **Re-sort tables stored sorted by a reference** (`findGroupingColumn` / + `resortByGroupingColumn`, Section 8). The same family: `O2fwdtrkcl` and + `O2trackqa_003` were coming out unsorted in every DF of two of the three + sample files, which breaks O2's slicing the same way the split `-1` group + did. Derived from the data, not from a list — see resolved gap 1. Maurice's #2418 has the same shape (defer `O2fwdtrack`, remap via the MFT/Fwd perms) and is correct; this generalises it from one table to all of them. @@ -331,13 +336,12 @@ row whose `fIndexMcCollisions` pointed to a dropped row is also dropped. These were identified during the refactor but not yet implemented: -### 1. Tables SORTED BY a reference into a reordered table are not re-sorted +### ~~1. Tables SORTED BY a reference into a reordered table are not re-sorted~~ (RESOLVED) -**This is the remaining member of the O2-7098 family — open, and the natural -follow-up PR.** Stage 1b reorders `O2track_iu` / `O2mfttrack` / `O2fwdtrack`. -Several other tables are *stored sorted by* a reference into one of those (or -into `O2collision`). Their values are now correctly remapped, but their rows are -left where they were, so the ordering is destroyed: +Stage 1b reorders `O2track_iu` / `O2mfttrack` / `O2fwdtrack`. Several other +tables are *stored sorted by* a reference into one of those (or into +`O2collision`). Their values were remapped but their rows left where they were, +so the ordering was destroyed: | table | key | sorted in input? | |---|---|---| @@ -347,16 +351,22 @@ left where they were, so the ordering is destroyed: | `O2v0_002`, `O2cascade_001`, `O2decay3body` | `fIndexCollisions` | yes | | `O2mfttrackcov` | `fIndexMFTTracks` | **no** — so not an invariant there | -(measured on `example_AOD/AO2D_pre.root`.) This is the same failure mode as the -split `-1` group that produced the `ArrowTableSlicingCache::validateOrder` -FATAL — not yet observed in the wild, but structurally identical. - -**Suggested fix** — data-derived rather than another hardcoded list: *if `T.B` -was sorted in the input and `B`'s referent was reordered, re-sort `T` by the -remapped `B`.* That rule reproduces Stage 1b's behaviour and correctly leaves -`O2mfttrackcov` alone. It is deliberately **not** in the O2-7098 fix: it changes -the row order of tables that are currently untouched, so it deserves its own -review. `AODBcRewriterCheckLinks` is already in place to police it. +(measured on `example_AOD/AO2D_pre.root`.) Same failure mode as the split `-1` +group that produced the `ArrowTableSlicingCache::validateOrder` FATAL. **Not +theoretical**: on `example_AOD/AO2D_pre.root` and `bigger2/`, `O2fwdtrkcl` and +`O2trackqa_003` came out unsorted in *every* DF. + +**Fix**: `findGroupingColumn` + `resortByGroupingColumn` (Section 8), driven by +the data rather than by another hardcoded list — *if `T.B` is non-decreasing in +the input and `B`'s referent gets reordered, re-sort `T` by the remapped `B`*. +Self-maintaining across schema changes, and it correctly leaves `O2mfttrackcov` +alone (its `fIndexMFTTracks` is not sorted on input, so there is no ordering to +preserve). Iterated to a fixed point, because these tables reference each other: +`O2cascade_001` is sorted by `fIndexV0s` and `O2v0_002` may itself have just +been re-sorted. + +Policed by a new check in `AODBcRewriterCheckLinks`: a column sorted on input +must be sorted on output. ### ~~2. `fIndexCollisions` inside `O2mccollision` is not remapped~~ (MOOT) diff --git a/MC/utils/tests/makeTestAOD.C b/MC/utils/tests/makeTestAOD.C index a437328f8..e87cf02dc 100644 --- a/MC/utils/tests/makeTestAOD.C +++ b/MC/utils/tests/makeTestAOD.C @@ -110,21 +110,31 @@ const int kFwdMatch[] = {-1, 0, -1, -1, 3, -1};// fwd -> fwd self-reference // exactly the metric O2-7098 was reported with. const int kFwdMcPart[] = {0, 3, -1, 6, 9, -1}; +// V0s and cascades are stored SORTED BY fIndexCollisions, as reconstruction +// writes them. The collision table gets reordered, so these have to be +// re-sorted or the grouping is destroyed. The cascades are additionally sorted +// by fIndexV0s, whose referent is itself one of these re-sorted tables — a +// two-level dependency that a single pass would not resolve. const int kNV0 = 2; const int kV0Coll[] = {0, 2}; const int kV0Pos[] = {0, 4}; const int kV0Neg[] = {1, 5}; -const int kNCasc = 1; -const int kCascColl[] = {0}; -const int kCascV0[] = {0}; -const int kCascBach[] = {2}; +const int kNCasc = 2; +const int kCascColl[] = {0, 2}; +const int kCascV0[] = {0, 1}; +const int kCascBach[] = {2, 4}; +// Sorted by fIndexTracks / fIndexFwdTracks respectively — both referents are +// reordered by Stage 1b. const int kNAmb = 2; const int kAmbTrack[] = {3, 6}; const int kAmbBCFirst[] = {0, 3}; const int kAmbBCLast[] = {2, 4}; +const int kNFwdCl = 4; +const int kFwdClTrack[] = {0, 1, 3, 4}; + } // namespace void makeTestAOD(const char *outFileName = "AO2D_test.root") @@ -308,6 +318,16 @@ void makeTestAOD(const char *outFileName = "AO2D_test.root") t.Write(); } + // ---- O2fwdtrkcl (grouped by fIndexFwdTracks, a Stage-1b table) --------- + { + TTree t("O2fwdtrkcl", "fwd track clusters"); + int fwd; float x; + t.Branch("fIndexFwdTracks", &fwd, "fIndexFwdTracks/I"); + t.Branch("fX", &x, "fX/F"); + for (int i = 0; i < kNFwdCl; ++i) { fwd = kFwdClTrack[i]; x = 3000.f + i; t.Fill(); } + t.Write(); + } + // ---- O2ambiguoustrack (slice into the deduplicated BC table) ------------ { TTree t("O2ambiguoustrack", "ambiguous tracks"); diff --git a/MC/utils/tests/testAODBcRewriter.C b/MC/utils/tests/testAODBcRewriter.C index a0357aa82..aa9e524ee 100644 --- a/MC/utils/tests/testAODBcRewriter.C +++ b/MC/utils/tests/testAODBcRewriter.C @@ -263,6 +263,40 @@ int testAODBcRewriter(const char *inFileName = "AO2D_test.root", " daughter slices still point at the right particles"); } + // ---- Grouping preserved for tables stored sorted by a reference --------- + // These are written sorted by an index into a table the rewriter reorders. + // Remapping the values without re-sorting the rows leaves the column + // unsorted, which breaks O2's slicing cache the same way a split "-1" group + // does. The referenced tables here are all genuinely reordered, so a + // no-op implementation cannot pass this by accident. + { + const char *cases[][2] = { + {"O2v0_002", "fIndexCollisions"}, + {"O2cascade_001", "fIndexCollisions"}, + {"O2cascade_001", "fIndexV0s"}, // referent is itself re-sorted + {"O2fwdtrkcl", "fIndexFwdTracks"}, + {"O2ambiguoustrack", "fIndexTracks"}, + }; + for (auto &c : cases) { + auto vIn = readInt(get(din, c[0]), c[1]); + auto vOut = readInt(get(dout, c[0]), c[1]); + auto ordered = [](const std::vector &v) { + Int_t prev = -1; bool seenNull = false; + for (auto x : v) { + if (x < 0) { seenNull = true; continue; } + if (seenNull || x < prev) return false; + prev = x; + } + return true; + }; + std::string what = std::string(c[0]) + "." + c[1]; + if (vIn.empty()) { fail(what + ": column missing from input"); continue; } + if (!ordered(vIn)) { fail(what + ": fixture is not sorted on input — fix makeTestAOD.C"); continue; } + if (!ordered(vOut)) fail(what + ": sorted on input, unsorted on output (grouping destroyed)"); + else pass(what + ": still sorted after the rewrite"); + } + } + // ---- Slice into the deduplicated BC table ------------------------------- { auto bcIn = readULong(get(din, "O2bc_001"), "fGlobalBC");