rexminnis commented on PR #2620: URL: https://github.com/apache/iceberg-rust/pull/2620#issuecomment-5304435818
@CTTY I'd like to help get this landed — while integrating in-process compaction we confirmed the exact failure mode this PR prevents, and I have test evidence + two commits to offer. **Why this matters concretely:** without `RewriteFiles`, the only way to commit a compaction from iceberg-rust is `fast_append` — and that doubles every rewritten row: the merged file is added while the source files stay referenced by the new snapshot, and `expire_snapshots`/`remove_orphans` can never clean them because they're live data, not history. We verified this directly: a 4-file/200-row table "compacted" via fast_append reads back as 400 rows, while the run reports success. `RewriteFilesAction` is the correct primitive for this, so I took this branch for a spin. **Where the branch stands:** the current head's `ManifestFilterManager::filter_manifests` is still the documented v1 placeholder (pass-through), so the branch can't yet commit a correct rewrite — removals no-op. I've filled in the deferred body, restoring the behavior @brgr-s live-tested on the earlier `rewrite poc` lineage, adapted to the current MSP architecture. **What I have on a branch** ([`rexminnis:drls/rewrite-files-revival`](https://github.com/apache/iceberg-rust/compare/main...rexminnis:iceberg-rust:drls/rewrite-files-revival), happy to PR it into your branch or hand it over however you prefer): 1. The real manifest filtering on the current MSP architecture: untouched manifests pass through verbatim; affected manifests are rewritten with removed entries dropped and survivors re-emitted as EXISTING (original snapshot id / sequence numbers preserved); fully-emptied manifests omitted. Includes @brgr-s's `fail_missing_delete_paths` (Java `failMissingDeletePaths` parity) threaded through the MSP — `RewriteFilesAction` sets it, since committing the add-half of a rewrite while the remove-half silently no-ops is precisely the duplication bug. Manifests written under a non-default partition spec are refused loudly (`FeatureUnsupported`) rather than rewritten with the wrong spec — lifting that needs a writer parameterized by the source manifest's spec. Unit tests cover filtering, the previously-missing missing-file case, and the spec-mismatch refusal; `MemoryCatalog` e2e tests drive full `Transaction` commits (replace-serves-only-merged, missing-source refusal, add-only rejection). 2. A snapshot-summary bug found during live testing: `SnapshotProducer::summary()` feeds only added files into `SnapshotSummaryCollector`, so a `replace` snapshot's totals over-count everything removed — my 4-file/200-row compaction committed with `total-data-files=5, total-records=400`. Totals chain from the previous summary, so once wrong they stay wrong for the table's lifetime. Fixed with a `removed_files()` hook on `SnapshotProduceOperation` (default empty) feeding `SnapshotSummaryCollector::remove_file`, and the e2e test pins the totals. **Live verification** (beyond the in-repo tests): built a small runner against this branch and ran it end to end — Apache Polaris REST catalog (OAuth + vended S3 credentials), S3 warehouse, 4 small parquet files → one merged file → `RewriteFilesAction` commit. Verified through Trino 483 (Iceberg Java 1.11) reading the resulting metadata: correct row count and column aggregates, 1 live file, `operation=replace`, and exact summary totals (`total-records=200, total-data-files=1, deleted-records=200, deleted-data-files=4`). So the metadata this branch writes is accepted and correctly interpreted by the Java implementation. On the open design question (cross-retry filter cache): the single `Mutex<MergingCache>` shape you kept gives the deferred cache a home without blocking v1 — always-rewrite-per-attempt is correct, just not optimal, and that seems fine to defer exactly as the code comments say. Two smaller things I noticed while in there, fine as follow-ups: staged `added_delete_files` on the MSP never reach the `SnapshotProducer` in `commit()`, and `delete_entries` currently returns empty (removed entries are dropped rather than written with DELETED status — worth a decision for incremental-consumer semantics, matching what the conflict-detection RFC settles on). Happy to rebase/split/reshape any of this to fit how you want the PR to move — and to keep running it against our Polaris/Trino setup as it evolves. -- 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]
