qzyu999 commented on PR #3131: URL: https://github.com/apache/iceberg-python/pull/3131#issuecomment-5337222247
Hi @kevinjqliu, @geruh, thanks for reopening the PR. Rebased and updated now that #3320 has merged. Here are the latest changes: **Integration with commit retry/validation (#3320):** - Added `_validate_concurrency()` override on `_RewriteFiles` that matches Java's `BaseRewriteFiles.validate()`, only checks `validateNoNewDeletesForDataFiles`, does NOT reject concurrent appends (those are not conflicts for a replace) - Added `_refresh_for_retry()` that clears the `@cached_property` so manifest entries are recomputed against the refreshed parent snapshot on retry CC: @lawofcycles, congrats on completing #3320, PTAL if you have time for these changes here. I believe that I've built in accordance with what we had discussed previously. **Other changes since last review:** - Refactored `_existing_manifests` into a shared `_get_existing_manifests()` method on `_SnapshotProducer` (used by both `_OverwriteFiles` and `_RewriteFiles`) - Changed validation errors from `ValueError` to `ValidationException` for consistency with the rest of the validation infrastructure **Test coverage (18 tests, all passing across memory/sql/sql_without_rowcount):** - Core replace: snapshot metadata, summary counts, manifest entry status/sequence numbers - Manifest reuse for unaffected files - Delete-manifest passthrough (v2) - No-op on empty input (both empty and non-empty tables) - Missing-file abort (`ValidationException`) - Records invariant violation (`added > deleted` -> `ValidationException`) - Soft-delete shrinking (`added < deleted` -> success) - Multi-file, partitioned table, custom branch - Concurrent non-conflicting append in different partition -> retry succeeds - Concurrent replace on different partitions -> both land - Concurrent append to same unpartitioned table -> NOT a conflict (key Java-parity behavior) - Concurrent delete of same file -> `ValidationException` - `_refresh_for_retry` clears cached state - Add-only replace raises invariant violation **Scoped to data-file rewriting only.** Delete-file rewriting, `dataSequenceNumber` override, and `validateFromSnapshot` are documented as future additive extensions in the class docstring. These exist in the Java version and should be readily extendable without major changes to the existing code structure. -- 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]
