fix: make uproot.recreate physically truncate an existing file - #1697
Draft
ariostas wants to merge 1 commit into
Draft
fix: make uproot.recreate physically truncate an existing file#1697ariostas wants to merge 1 commit into
ariostas wants to merge 1 commit into
Conversation
Before the fsspec migration (scikit-hep#1016, scikit-hep#1034), uproot.recreate opened the path with mode "w", truncating it. Afterwards the truncation moved into FileSink, which only calls _truncate_file when the file does *not* already exist, so recreate over a pre-existing file opened it "r+b" and left every byte past the new fEND in place. Recreating a 100 kB path as an empty ROOT file left the physical size at 100 kB with fEND at 1658. Truncate the path in recreate, restoring the documented "RECREATE" semantics. uproot.update is unaffected and keeps its "r+b" behavior, and uproot.create still raises FileExistsError before reaching this point. Recreating from a file-like object is unchanged, as it was before the migration too. Assisted-by: claude-code:claude-opus-5[1m]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Addresses finding 3 of "PR 1" in #1688. This is a regression, with a clear origin.
Before the fsspec migration,
uproot.recreatetruncated the path itself:#1016 / #1034 moved that into
FileSink.__init__, which only truncates when the file does not already exist:https://github.com/scikit-hep/uproot5/blob/main/src/uproot/sink/file.py#L52-L58
so recreating over an existing file opens it
"r+b"and leaves every byte past the newfENDin place. Recreating a 100 kB path as an empty ROOT file left the physical size at 100 kB withfENDat 1658.Restores the truncation in
recreate, which is also the pathcreatedelegates to.uproot.updateis unaffected and keeps its"r+b"behavior.uproot.createstill raisesFileExistsErrorbefore reaching this point.Tests
tests/test_1688_recreate_truncates.py: physical size after recreating over a large non-ROOT file and over a larger ROOT file, plus guards thatrecreatestill creates missing files and parent directories, thatcreatestill refuses to overwrite without touching the file, and thatupdatedoes not truncate. 2 of the 5 fail onmain.Full suite passes locally (1028 passed, 90 skipped).