Repository navigation
Conversation
- Return explicit errors instead of silently defaulting to 0 for sequence_number and file_sequence_number in manifest entries - Use standard manifest naming scheme (<uuid>-m<counter>.avro) instead of custom names (-m-filter-, -m-added) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`ManifestFilterManager::filter_manifests` built each surviving entry with status Existing and the original snapshot id and sequence numbers, then handed it to `ManifestWriter::add_entry`, which rewrites the status to Added, replaces the snapshot id with the new snapshot's, and clears the file sequence number. Survivors of a rewrite were therefore recorded as newly added files of the rewrite snapshot. Use `add_existing_entry`, which only sets the status and leaves the ids the filter already resolved, and drop its `#[allow(dead_code)]`.
Java's `RewriteFiles.dataSequenceNumber` lets a compaction record the sequence number of the files it replaces, so the rewritten file does not shadow deletes written concurrently at a higher sequence number. Without it the added files inherit the rewrite snapshot's own sequence number. `RewriteFilesAction::data_sequence_number` sets it, and `write_added_manifest` then writes the added files through `ManifestWriter::add_file`, which records the given number on the entry. A number above the one the new snapshot will carry is rejected at commit; on a V1 table that is 0, matching the V1 entry layout, which records no sequence number at all. The parameter is `i64`, not `u64`, to match `ManifestWriter::add_file` and every other sequence number in the crate.
`commit_snapshot` handed `SnapshotProducer` an empty added-file list and went straight to `commit_with_manifests`, so neither `validate_added_data_files` nor `validate_duplicate_files` ever saw the files a rewrite adds. A file carrying a partition value the default spec cannot hold, or a path that is already live, was written into a manifest and committed. Pass the added files to the producer and run both validations first, and drop `write_added_manifest`'s own content-type check, which `validate_added_data_files` now makes unreachable.
`commit_snapshot` created a fresh `(0..)` counter on every call while the
commit uuid stayed fixed for the life of the action, so a retried commit
rewrote `{uuid}-m0.avro` over the manifest the previous attempt had
written under the same name.
Java keeps `manifestCount` on the snapshot producer and never resets it.
The counter cannot live on Rust's `SnapshotProducer`: that is built fresh
inside each `commit_snapshot` call and so resets with it. The object that
survives a retry is the action, which the transaction re-invokes through
an `Arc`, so the counter goes on `MergingSnapshotProducer` as an
`AtomicU64` — `commit` only has `&self`. `fast_append` is unaffected: it
draws a new commit uuid per attempt unless the caller pins one.
The filter carried a manifest forward on `has_deleted_files()`, so a manifest whose entries are all Deleted stayed in every following snapshot for the rest of the table's life. Java keeps an existing manifest on `hasAddedFiles() || hasExistingFiles()`; its third arm matches the snapshot being produced, which no manifest read from the current snapshot can carry. The manifests this commit rewrites are produced by the filter pass and kept on their own.
`starting_snapshot_id` was recorded and never read. A rewrite planned against one snapshot and committed against a later one silently drops any delete written in between: the replacement file keeps the deleted rows. Java resolves this per file in `validateNoNewDeletesForDataFiles`, which needs a delete-file index this crate does not have yet. Until then the commit fails closed when the current snapshot carries a delete manifest with a sequence number above the starting snapshot's.
Every rewrite test used the V3 fixture, so nothing covered the V2 manifest and entry layout the action also writes. Both cases now run on V2 as well, through a V2 catalog fixture alongside the V3 one.
`cargo fmt` over the two new modules, and `public-api.txt` regenerated for `Transaction::rewrite_files`. The clippy warning on the unnecessary parentheses around the manifest counter went with the counter itself.
Resolves two conflicts from the transaction module re-export refactor on main: action types are now re-exported at `iceberg::transaction::*`, so `RewriteFilesAction` is exported the same way and `public-api.txt` records it under the flat path. Co-authored-by: Jegor van Opdorp <jegorvanopdorp@gmail.com>
V1 manifest entries carry no sequence numbers, and `ManifestEntryV1` deserialization fills both in as 0. Nothing in the rewrite path treats that as missing, so a V1 table compacts like V2 and V3 and the surviving entries keep Existing status, their snapshot id and their zero sequence numbers. Cover it so it stays that way.
`validate_added_data_files` is shared by fast append and rewrite, but its content-type error said "for fast append", so a rewrite that was handed a delete file reported the wrong operation and not which file was wrong. Drop the operation from the message and name the file and its content type instead.
`validate_no_new_deletes` fell back to sequence number 0 when the starting snapshot was no longer in the table, which let a rewrite commit when the table held no delete manifest at all and otherwise blamed an unrelated delete manifest for the rejection. An expired starting snapshot means no delete can be ruled out, so say that instead.
`validate_no_new_deletes` skipped the check whenever either the starting snapshot id or the current snapshot was absent. A missing starting snapshot id means the table held nothing when the rewrite was planned, so everything it holds at commit time, delete files included, arrived after that -- the case the check is least able to skip. Reject it, and keep returning early only when the table still has no snapshot, where the filter rejects the rewrite for its missing sources anyway.
The conflict check is table-wide and always on, so a compaction fails on a delete committed anywhere in the table while it ran, not only on the files it replaces. Say so on the action, along with the two other cases that cannot rule out a delete and what the check does not cover, so a caller can tell an expected rejection from a bug. Also stop the public docs linking to the private MergingSnapshotProducer, which rustdoc drops on the generated page.
The macro from apache#2928 landed while this branch was open and snapshot.rs already uses it. Use it for the merging producer and the rewrite action too, so the whole transaction module reads the same way.
The filter resolves each manifest's own partition spec, but every test ran on the default spec. Add an identity(ts) -> day(ts) fixture on V2 and V3: a survivor stays Existing in a spec-0 manifest with its timestamptz value and partition summary intact, one rewrite filters a manifest of each spec, and retiring the last old-spec file drops the spec-0 manifest with the chained totals still exact.
build_summary merged the filter's removal collector into a fresh one; merge drops partition metrics unless both sides trust them and neither did, so a rewrite never reported changed-partition-count or the partitions.* summaries. Add the new files to the filter's collector instead, and honour write.summary.partition-limit as fast append does.
hotcache
force-pushed
the
feat/rewrite-files-action
branch
from
September 28, 2026 23:41
e7db3c8 to
ebf1946
Compare
Owner
|
Merged — both commits are now on Sorry this PR got a bit messy. Shortly after you opened it, I rewrote the branch so all the commits use the same identity. Some were authored as The actual trees were byte-for-byte identical, but the rewrite changed all the SHAs from the merge commit onward, including the Nothing you did caused this, and there’s nothing you need to redo. I rebased your two commits onto the new tip and they applied cleanly. I also took care of the An append keeps the empty manifest around, same as Java, since only a merging commit rebuilds the manifest list. So the test does a second rewrite instead of an append. More details in my reply on apache#3046. Thanks again for the fixture and for running it against Polaris. Finding those missing |
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.
The fixture offered on apache#3046: an
identity(ts)→day(ts)table on V2 and V3.test(transaction): cover rewrites on a spec-evolved table— a survivor staysExistingin its spec-0 manifest with its timestamptz value and partition summary intact; one rewrite filters a manifest of each spec; retiring the last old-spec file drops the spec-0 manifest with chained totals exact. All pass on the branch as-is.fix(transaction): keep partition metrics in the rewrite summary—build_summarymerged two collectors that don't trust partition metrics, so replace snapshots never reportedchanged-partition-countorpartitions.*. Adds the new files to the filter's collector instead and honourswrite.summary.partition-limit. Its test fails without the fix.cargo test -p iceberg --lib(1817),cargo fmt --checkandclippy -D warningsare clean; no public API changes.