fix(overwrite): address 5 correctness issues from post-merge review#77
Merged
Conversation
…view 1. snapshot.rs: fix `previous_snapshot` lookup — `self.snapshot_id` is the new (uncommitted) snapshot and is never in the metadata; use `current_snapshot()` instead to correctly compute the previous totals for `update_snapshot_summaries`. This fixes a latent u64 underflow when a delete-only overwrite results in a net-negative file count. 2. overwrite.rs: `rewrite_manifest` now looks up the partition spec from `manifest_file.partition_spec_id` instead of always using the default spec, preventing partition-metadata corruption on spec-evolved tables. 3. overwrite.rs: guard against delete-only overwrite on a table with no current snapshot — return `PreconditionFailed` instead of silently producing a misleading snapshot. 4. overwrite.rs: `existing_manifest` now preserves delete-only manifests (`has_deleted_files()`) in both filter paths, matching the behavior of `FastAppendOperation` and maintaining time-travel auditability. 5. Two new tests: `test_overwrite_unaffected_manifest_passthrough` covers the passthrough branch (manifest with no matching deletes), and `test_delete_only_overwrite_summary` covers correct summary computation for a delete-only overwrite (regresses the u64 fix in point 1). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
Addresses 5 correctness issues flagged in the post-merge review of #76.
Fix 1 (snapshot.rs):
previous_snapshotlookup usedself.snapshot_id(the new, uncommitted snapshot — never in the metadata), so it always resolved toNone. Changed totable_metadata.current_snapshot(). This fixes a latent u64 underflow when a delete-only overwrite has a net-negative file count.Fix 2 (overwrite.rs):
rewrite_manifestalways useddefault_partition_spec()when writing the rewritten manifest. Now looks uppartition_spec_by_id(manifest_file.partition_spec_id), returningDataInvalidif missing. Prevents partition-metadata corruption on spec-evolved tables.Fix 3 (overwrite.rs): A delete-only overwrite on a table with no current snapshot silently produced a misleading snapshot. Now returns
PreconditionFailedupfront.Fix 4 (overwrite.rs):
existing_manifestdropped delete-only manifests in both filter paths, whileFastAppendOperationwas just widened (in feat(transaction): add OverwriteAction with CoW delete support #76) to preserve them. Addedhas_deleted_files()to both paths for consistency and to preserve time-travel auditability across overwrite chains.Fix 5 (overwrite.rs): Added two tests:
test_overwrite_unaffected_manifest_passthrough(exercises the passthrough branch for manifests with no matching deletes) andtest_delete_only_overwrite_summary(regresses Fix 1 — would panic with u64 underflow without it).Test plan
cargo test -p iceberg --lib transaction::overwrite::tests— all 7 tests passcargo test -p iceberg --lib transaction::— all 26 transaction tests pass🤖 Generated with Claude Code