u70b3 commented on PR #2185:
URL: https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885503894
Hi @glitchy — I did a deeper review of the overwrite path on the latest
branch, including regression tests and cross-validation against the Iceberg
Java SDK + spec. Below is the consolidated summary, including one item I
withdrawn.
## Real bugs (with a follow-up PR)
**1. `rewrite_manifest` uses the table's current schema / default partition
spec** (`crates/iceberg/src/transaction/overwrite.rs`)
The `ManifestWriterBuilder` is built from
`table.metadata().current_schema()` and `default_partition_spec()`. After
schema or partition evolution this writes a manifest whose schema-id /
partition-spec-id do not match the original entries. The spec requires a
manifest to keep its own schema and spec: a manifest's schema is based on its
own partition spec (`spec.md` L663, L741), scan planning uses the manifest's
spec "regardless of the current partition spec" (L1053), and the manifest
list's `partition_spec_id` must match (L1010). Java agrees —
`SnapshotProducer.newManifestWriter` uses `reader.spec()` (the original
manifest's spec) plus the table's current format version
(`SnapshotProducer.java:632-639`, `ManifestFilterManager.java:506`). Fix: use
`manifest.metadata().schema` / `manifest.metadata().partition_spec`.
**2. Already-`Deleted` entries are flipped back to `Existing` on rewrite**
(`OverwriteOperation::rewrite_manifest`)
The rewrite loop only sends `entry.is_alive() &&
deleted_file_paths.contains(...)` through `add_deleted_entry`; entries already
marked `Deleted` fall through to `add_existing_entry`, changing their status
back to `Existing`. Java's
`ManifestFilterManager.filterManifestWithDeletedFiles` iterates
`reader.liveEntries()`, which filters out `Status.DELETED`
(`ManifestFilterManager.java:509`, `ManifestReader.java:354-365`) — i.e. Java
*drops* already-Deleted entries on rewrite rather than re-emitting them. Fix:
match Java — `if !entry.is_alive() { continue; }`. (An earlier draft re-emitted
them as Deleted with an overwritten `snapshot_id`, which would lose the
original deletion's provenance; dropping is the correct, Java-aligned fix.)
I opened a small follow-up PR with both fixes + regression tests:
https://github.com/glitchy/iceberg-rust/pull/3. Tests: `cargo test -p iceberg
transaction::overwrite --lib` (7 passed) and `transaction::append --lib` (7
passed).
## Withdrawn (my mistake)
**Overwrite drops delete-only manifests → files reappear.** I initially
flagged this as P0 but it is **not a bug**. Java drops delete-only manifests
too, via `MergingSnapshotProducer.shouldKeep = hasAddedFiles() ||
hasExistingFiles() || snapshotId == current`
(`MergingSnapshotProducer.java:1003-1007`), and the Rust reader already skips
`Deleted` entries at scan time (`crates/iceberg/src/scan/mod.rs:516` via
`ManifestEntry::is_alive()`). A delete-only manifest carries no information not
already captured by the absence of those files in older snapshots, so dropping
it does not cause files to reappear. Apologies for the noise on this one — the
corresponding code in the follow-up PR keeps Java's drop behavior.
## Out of scope (not in the follow-up PR)
A few additional issues surfaced locally but are out of scope for that small
PR: partial-overwrite summary treated as full-table truncate
(`SnapshotProducer::summary` passes `truncate_full_table=true` for every
`Operation::Overwrite`, which corrupts `total-*`/`deleted-*` and can cascade
into an `update_totals` underflow panic on a later overwrite), summary
`update_totals` doing unchecked `previous_total + added - removed` u64
subtraction (`snapshot_summary.rs:535`), non-existent delete files being
silently accepted and counted in the summary, duplicate delete paths, no
add/delete path-intersection check, and the empty-action error message not
mentioning `deleted_data_files`. Happy to file these as a separate follow-up if
useful.
## One false positive (for the record)
V1 overwrite does **not** fail on sequence numbers. V1 manifest entries are
parsed with `sequence_number = Some(0)` (`spec/manifest/_serde.rs:90-91`), so
`add_deleted_entry`'s `add_entry_inner` check passes and a V1 overwrite-delete
commits successfully. I mention this only to save anyone else from the same
wrong assumption.
Happy to rebase the follow-up PR or split it differently if you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
