wirybeaver commented on PR #2203: URL: https://github.com/apache/iceberg-rust/pull/2203#issuecomment-4257283323
Thanks for the thorough reviews @jdockerty and @DAlperin! Summarizing what's been updated since the initial review: **Code fixes** (latest commits after the initial submission): - `existing_manifest()`: now properly **excludes manifests containing deleted files** rather than carrying all of them forward. In CoW mode, when a data file is rewritten, any manifest referencing the old file must be excluded from the new snapshot — otherwise the table would still expose the old file to readers. - `delete_entries()`: builds `ManifestEntry` with `ManifestStatus::Deleted` and the correct snapshot ID. - `write_delete_manifest` error message typo fixed: "write" → "writing". - `test_row_delta_validate_from_snapshot`: changed to `unwrap_err().kind()` asserting `ErrorKind::DataInvalid` instead of string matching. - `operation()` doc comment: removed the inaccurate "Only adds delete files → Delete" bullet; added explicit note that `Operation::Delete` is deferred until Merge-on-Read is wired up. - Added inline comment explaining why `removed_data_files` are not validated (they are already-committed table files; matches Java `MergingSnapshotProducer` behavior). **PR description** has been rewritten for reviewers unfamiliar with the CoW/MoR distinction — includes a background section, method table, and links to the specific Java classes this port is based on. -- 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]
