hotcache commented on PR #3046: URL: https://github.com/apache/iceberg-rust/pull/3046#issuecomment-5892867580
Thanks @rexminnis — both of your commits are in, and I took the `DELETED` entry change as well. **Merged from hotcache/iceberg-rust#2**, with your authorship: * `43a09a5` — the spec-evolution fixture. It passes as-is, so the filter resolving each manifest against its own spec works fine on an evolved table. * `89786a8` — the partition-metrics fix. `merge` dropping the metrics unless both sides trust them is a bit of a sharp edge. Keeping the fix local to the rewrite instead of changing the default in `snapshot_summary.rs` seems right to me. I added the shared default to the follow-ups in the description so it doesn't get forgotten. I closed the PR by hand since GitHub didn't close it automatically. The 19 commits it was showing were my fault: after you opened it, I rewrote the branch so all commits use the same identity. Some were authored as `abel`, which is the same person as `hotcache` but shows up as a different name in the commits. That would have looked like a third contributor here. The trees were byte-identical, but the rewrite changed all the SHAs from the merge commit onward, including `e7db3c8`. Your two commits rebased cleanly onto the new tip, so there's nothing you need to redo. ### Removed files are now `DELETED` entries — `8a4f780` Took this as described. `filter_manifests` now writes each removed entry with `add_delete_entry`, which stamps the removing snapshot while keeping the file's own sequence numbers. When a commit empties a manifest, we keep the manifest with just those deleted entries rather than dropping it immediately. The next merging commit removes it. This is the same rule `82ad3ae` already used for manifests that became all-deleted in an earlier snapshot. A few assertions needed to move with that behavior, so you don't need to change the fixture: * `test_rewrite_files_removes_empty_manifest` and `retires_the_old_spec` no longer expect the manifest to be absent. They check that it has no live entries and that a **later rewrite** drops it. My first attempt used an append and failed because the all-deleted filtering happens in `MergingSnapshotProducer`, not `FastAppend`. So an append keeps the manifest around, just like Java does. Only a merging commit rebuilds the manifest list. That's #2545 again. * `keeps_survivors_on_their_partition_spec` now expects the spec-0 manifest's partition summary to include the removed file as well as the survivor (`lower_bound` is the morning value rather than the evening one). This matches Java's `ManifestWriter#addEntry`, which updates the summary for every entry regardless of status. Reverting the change makes both of the first two tests fail, so they should help lock in the behavior. `transaction::rewrite` is now at 25 tests. `cargo test -p iceberg`, `cargo fmt`, `clippy -D warnings`, `cargo doc -D warnings`, and `make check-public-api` are all clean. ### Still open The interim conflict check is unchanged and still table-wide. The file-level `validateNoNewDeletesForDataFiles` is still PR6, on #2243. Partition-summary pruning, `set_commit_uuid` / snapshot properties, and the `SnapshotSummaryCollector::merge` default are all listed as follow-ups in the description. Thanks again for running this against Polaris with an independent reader. The missing `DELETED` entries weren't something the unit tests could catch — the summary reported `deleted-data-files=2`, and nothing looked wrong until Trino reported `deleted_data_files_count = 0`. -- 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]
