feat: Add in-place TTree branch addition and row extension - #1690
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
|
ariostas
left a comment
There was a problem hiding this comment.
This is looking really promising! Thanks for the hard work!
I left some comments for specific locations.
But one more general comment is that this seems to be doing a lot of manual scanning and patching. It would be better to try to do what you did for the RNTuple one. Deserializing what you need to construct a WritableTree (with self._cascading properly built) and then let the existing functionality take care of as much of the serialization as possible.
…f numpy_dtype, preventing silent data corruption when extending an existing TTree via uproot.update()
… content-independent byte markers instead of searching for zero-valued fields, preventing file corruption when extending a tree with zero baskets
…ing AsStrings interpretation in _load_existing_ttree
…stead of crashing on access, and make the add_branches/extend TBranchElement guards actually reachable
…e search, support fixed-size array branches, and reject divergent per-branch basket counts
…ead of stamping them with the whole tree's basket count
…y-named n<branch> sibling
…ates the tree, instead of leaking a stale one
…y reloading and re-parsing the tree from disk
…h on a preexisting tree, fixing silent corruption when extending a ROOT-written TTree
…ta, and reject jagged accept_new_fields cleanly
… default field_name convention when reopened via uproot.update()
…stent tree, not just Exception
…th a nonexistent branch, not just Exception
ariostas
left a comment
There was a problem hiding this comment.
Thanks for all the hard work, @Yokubas! This is great! It's awesome that not only did you finish the project Zoe was working on, but you also ended up also adding the functionality to add more entries to the TTree, which we had not even planned.
Resolves conflicts with scikit-hep#1690 (in-place TTree branch addition and row extension), which landed on main and touches the same regions of writable.py. The two features are complementary rather than overlapping: - WritableDirectory._get: main routes preexisting TTrees to _load_existing_ttree, this branch routes preexisting RNTuples to _load_existing_ntuple. Both branches kept; each side's "cannot view preexisting" TypeError is now dead and removed. - The new methods between _get and _del: main added _load_existing_ttree, this branch added _read_ntuple_envelope and _load_existing_ntuple. Git interleaved them because they share boilerplate (sink.flush, _get_chunk, _ReadForUpdate). All three kept verbatim from their respective sides. Full test suite passes (1106 passed, 94 skipped), including both tests/test_1687_rntuple_update.py and tests/test_1690_ttree_inplace.py. Assisted-by: claude-code:claude-opus-5[1m]
Summary
Implements in-place modification of existing TTrees:
f["tree"].add_branches({"new_x": array1, "new_y": array2, ...})— add one or more new branches back-filled with provided dataf["tree"].extend({"x": array1, "y": array2})— append new entries to one or more existing branchesf["tree"].extend({"x": array1, "new_y": array2}, accept_new_fields=True)— auto-add new branches back-filled with zeros, then extendHow it works
Uses uproot's cascade machinery instead of manual byte patching:
extend: deserializes the existing TTree using uproot's reading side (branch members, cursor positions), reconstructs act.Treecascade object, then delegates to the existing cascade write machinery — which appends new baskets and patchesfBasketSeek,fBasketBytes,fBasketEntry,fWriteBasket,fEntryNumber, andfEntriesin the TTree blobadd_branches: creates new branch dict via_branch_np, callswrite_anewto rewrite the TTree metadata blob with the new branch included, then writes one basket per new branch. Existing basket data is never touched — the metadata blob just gains new branch headers and the basket seek arrays are updatedaccept_new_fields=True: callsadd_brancheswith zeros for existing entries, then extends with the provided data using the updated cascademetadata_start,basket_metadata_start) are derived structurally, by walking the same layoutwrite_anewitself emits (Tree._build_out()), rather than by searching the blob for byte patterns. The original byte-pattern-search approach broke whenever the value being searched for was0(e.g. a freshly created tree with no baskets yet, or a branch that had never been extended) — a search for zero bytes matches arbitrary unrelated data and silently corrupted the file. Deriving the offsets from the same code path that writes them removes that whole class of bug.Known limitations
particle.phi) is not yet implementedadd_branches/extendfor TBranchElement (split-object) files is not yet supported — this is now enforced with a cleanNotImplementedErrorand covered by tests; previously it could crash on plain access to such a file, or silently desynchronize entry counts across branchesadd_branches/extendrequire every branch to agree on basket count and capacity. This is always true for a tree Uproot itself wrote (and hasn't touched withadd_branchessince), but a ROOT-written tree commonly has divergent per-branch basket counts (branches flush baskets at different rates depending on per-entry size) — this case is now detected and rejected with a clearNotImplementedErrorrather than corrupting the file.Fixes made during review
Several data-corruption and crash bugs were found and fixed after the initial implementation, all confirmed with reproductions and covered by regression tests:
extendin update mode wrote garbage instead of the real values —_load_existing_ttreederived a jagged branch's dtype from the wrong place (the interpretation's ownnumpy_dtype, which isobjectfor a jagged array) instead of its content dtype.mktree'd, never extended) corrupted the file, for the same "search for zero bytes" reason described above._load_existing_ttreehad no handling for theAsStringsinterpretation.float[3]) crashed on plain access, not just extend —_load_existing_ttreedidn't account for a branch's shape.add_brancheson a tree that already had more than one basket wrote an incorrectfWriteBasketfor the new branch (the whole tree's basket count instead of the new branch's real count of 1), corrupting the new branch's layout.add_branchesleaked a stale cache entry inWritableFile._treeson every call after the tree relocated; now cleaned up via the same_move_treepathextend's basket-capacity growth already uses.add_branchesunconditionally re-read and re-parsed the whole tree from disk on every call even though the in-memory state was already current; now reuses it directly."id"incorrectly paired with an unrelated int branch"nid"); the counter-inference heuristic now excludes string branches.Tests (36 passing)
add_branchesandextendfor simple TBranch filesadd_branchescalls across separate sessionsadd_branchesthenextendin the same sessionextendcalls across separate sessionsextendafteradd_branchesin a new sessionaccept_new_fieldsbehavioruproot.update()session (regression test for the dtype-corruption fix)AsStringshandling)NotImplementedErrorfromextend/add_brancheson such files (regression tests)add_branchesafter multiple extends (regression test for thefWriteBasketcorruption fix), including the correct rejection of a follow-upextendonce basket counts divergeadd_branchesdoes not leakWritableFile._treescache entries (regression test)add_branchesdoes not re-read the tree from disk (regression test)