u70b3 commented on PR #2185: URL: https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885472835
Following up on my earlier comment (https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885293298) — after cross-validating the three items against the Iceberg Java SDK (`/home/kid/dev/iceberg`) and the Iceberg spec, I need to correct the record on one of them. **Item 1 (Overwrite drops delete-only manifests) — withdrawn, not a bug.** I previously flagged this as P0 with the rationale that dropped delete-only manifests would make removed files reappear as live data. That rationale was wrong. The Java SDK 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 that is not already captured by the absence of those files in older snapshots, so dropping it is safe. Apologies for the noise on this one. **Item 3 (Rewritten manifest uses current schema / default partition spec) — confirmed, real bug.** Matches Java: `SnapshotProducer.newManifestWriter` uses the original manifest's spec (`reader.spec()`) and the table's current format version. The spec also requires it (`spec.md` L663, L741, L1053, L1010). **Item 5 (Previously Deleted entries rewritten as Existing) — confirmed, real bug, fix realigned to Java.** The original concern stands: the pre-fix code sent already-Deleted entries through `add_existing_entry`, flipping them back to `Existing`. The fix now matches Java's `ManifestFilterManager.filterManifestWithDeletedFiles`, which iterates `reader.liveEntries()` and *drops* already-Deleted entries on rewrite (`ManifestFilterManager.java:509`, `ManifestReader.java:354-365`) rather than re-emitting them. (An earlier version of the fix re-emitted them as Deleted with an overwritten `snapshot_id`, which would have lost the original deletion's provenance — corrected.) I opened a small follow-up PR with the realigned fixes for #3 and #5, plus regression tests: https://github.com/glitchy/iceberg-rust/pull/3 The other items from my local review (summary truncate #2, summary underflow #6, silent non-existent deletes #7, etc.) remain open but are out of scope for that follow-up. -- 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]
