dina-kar commented on PR #2203: URL: https://github.com/apache/iceberg-rust/pull/2203#issuecomment-6331864253
Thanks for this PR. I tested it because we need RowDelta for merge-on-read deletes (deletion vectors on v3, position deletes on v2; see #2580). Here is what I found. **1. It conflicts with current `main`.** - #3253 already brought most of the `snapshot.rs` changes: the removed files on `SnapshotProducer`, `truncate_full_table`, and `new_manifest_writer` taking the manifest's schema and spec. - #3265 changed `TransactionAction` to the stateful trait. I adapted the PR to that API without changing its behaviour. It implements `StatelessAction`, reads manifests through `table.manifest_reader()`, and rewrites each manifest under its own spec. With that, all 7 of its tests pass. The adaptation is a single merge commit, if it helps: https://github.com/ostrium-labs/iceberg-rust/pull/4 **2. A bug: a second rewrite of the same manifest brings deleted files back.** - `existing_manifest` rewrites every entry of a manifest it touches, including entries an earlier snapshot already marked `Deleted`. Those come back as `Existing`. - To reproduce: fast-append `a`, `b` and `c` in one commit, then `row_delta().remove_data_files([a])`, then `row_delta().remove_data_files([b])`. After the second commit the scan returns `a` again. - The fix is to carry only live entries (`entry.is_alive()`) into the rewritten manifest. Java's `ManifestFilterManager` drops the deleted entries in the same way. - A regression test for this is `test_rewriting_a_rewritten_manifest_keeps_deleted_files_deleted` in https://github.com/ostrium-labs/iceberg-rust/pull/5. It fails without the fix. **3. `validate_from_snapshot` works differently from Java.** Here it rejects the commit whenever the table has moved past the given snapshot. In Java it sets the snapshot from which concurrent changes are checked for conflicts. This may be fine for now, but the name suggests Java's meaning. **4. Merge-on-read.** On top of this PR, https://github.com/ostrium-labs/iceberg-rust/pull/5 implements `add_delete_files` and a new `remove_delete_files`: - **Delete manifests.** Added deletion vectors (v3), position delete files (v2) or equality delete files go into a new delete manifest per partition spec, with inherited sequence numbers. - **Removing.** Removing a delete file rewrites its delete manifest. A DV is matched by Puffin path plus `content_offset`, because one Puffin file holds many DVs. - **Format checks.** v3 accepts only DVs as position deletes, v2 rejects DVs, and a data file can't end up with two live DVs. - **Operation and summary.** The operation follows `BaseRowDelta`: `Append`, `Delete` or `Overwrite`. The summary counts added and removed delete files. To test it, iceberg-rust committed DVs to three Spark 4.0.4 / Iceberg 1.12.0 v3 tables, replacing Spark's DVs, and position deletes to a v2 table. Spark, Trino 483, DuckDB 2.0.0 and iceberg-rust's reader then gave identical answers in all eight cases: row counts, `sum(id)`, and `_row_id` sums on v3. The integration tests against the Spark-provisioned tables are in https://github.com/ostrium-labs/iceberg-rust/pull/6. I'm happy to help however suits you: - send the fix in point 2 as a suggestion here; - open the merge-on-read part as a follow-up once this PR merges; - or put it into this PR, if you prefer. I'd also like to know how you see this fitting with #3041 (gating actions by format version) and the stateful-transactions work in #3266, so the merge-on-read part doesn't get in the way of either. -- 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]
