u70b3 commented on PR #2185: URL: https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885476842
Correction to my earlier reviews — apologies, this turned out to be partly a false alarm on my end. To be transparent about the back-and-forth: I first flagged three P0 items (https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885293298), then opened a follow-up PR with fixes (https://github.com/glitchy/iceberg-rust/pull/3), then corrected the record after cross-validating against the Java SDK and spec (https://github.com/apache/iceberg-rust/pull/2185#issuecomment-4885472835). The net outcome: - **#3 (rewrite manifest with current schema / default partition spec)** — real bug, fix stands. - **#5 (already-Deleted entries flipped back to Existing on rewrite)** — real bug, fix stands (realigned to Java's `liveEntries()` drop semantics). - **#1 (overwrite drops delete-only manifests)** — **my mistake, not a bug.** I withdraw it. Java drops delete-only manifests too, and the Rust reader already skips `Deleted` entries at scan time, so no data reappears. The "files would reappear as live data" rationale was wrong. Sorry for the churn on #1 — it should not have been called P0. The two real issues (#3, #5) and the follow-up PR (https://github.com/glitchy/iceberg-rust/pull/3) stand as the substantive feedback. -- 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]
