lawofcycles commented on PR #3320: URL: https://github.com/apache/iceberg-python/pull/3320#issuecomment-4762134391
> On `_SnapshotProducer._validate_concurrency`, this line: > > `conflict_detection_filter = self._predicate if self._predicate != AlwaysFalse() else None` > > When a producer has no predicate (_predicate == AlwaysFalse()), conflict_detection_filter becomes None, and under the default serializable isolation we then call: > > `_validate_added_data_files(table, catalog_head, None, starting_snapshot)` > > Since `_filter_manifest_entries` treats data_filter=None + partition_set=None as "match everything," this validates against every data file added anywhere in the table during the commit window. So a predicate-less overwrite gets aborted by any concurrent append — even one to a completely disjoint partition. > > The high-level paths are unaffected — delete()/overwrite(overwrite_filter)/upsert() all populate _predicate, so they scope correctly. The gap is the low-level update_snapshot().overwrite() + explicit delete_data_file() path (file/row-level overwrites that don't set a predicate), and presumably the _RewriteFiles integration discussed above — i.e. compaction/rewrite-style operations that touch a known, bounded set of files. > > For those, Java doesn't fall back to whole-table: RewriteFiles/OverwriteFiles derive the conflict scope from the operation's own added/deleted files (a partition set), so a file-level rewrite isn't invalidated by an unrelated concurrent append. With the current fallback, these operations become effectively un-retryable under any concurrent write on the table, which works against the PR's goal of making non-conflicting commits seamless. > > Two questions: > > 1. Is the whole-table fallback for predicate-less producers intentional for now (e.g. deferring scoped detection to the _RewriteFiles work), or worth addressing here? > 2. Would deriving a partition_set from the producer's _added_data_files/_deleted_data_files when _predicate is absent be a reasonable way to scope it, matching the Java behavior? Good analysis. You're right that predicate-less producers currently fall back to whole-table validation under serializable isolation. This doesn't affect the paths exposed by this PR since `delete()`, `overwrite()`, and `append()` all set `_predicate` before reaching `_validate_concurrency`. The gap is in the low-level `update_snapshot().overwrite()` + explicit file deletion path, and the future `_RewriteFiles` integration for compaction. Deriving a partition set from `_added_data_files`/`_deleted_data_files` when `_predicate` is absent (matching Java's approach) makes sense as the fix. This will become relevant when `_RewriteFiles` lands and the path becomes user-reachable. -- 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]
