rambleraptor commented on code in PR #3811:
URL: https://github.com/apache/iceberg-python/pull/3811#discussion_r3816138067
##########
pyiceberg/table/update/validate.py:
##########
@@ -195,6 +195,33 @@ def _validate_deleted_data_files(
raise ValidationException(f"Deleted data files were found matching the
filter for snapshots {conflicting_snapshots}!")
+def _validate_data_files_exist(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_files: set[DataFile],
+ parent_snapshot: Snapshot | None,
+) -> None:
+ """Validate that explicitly replaced data files have not been concurrently
deleted.
+
+ Args:
+ table: Table to validate
+ starting_snapshot: Snapshot at the end of the validation window
+ data_files: Data files that must still exist
+ parent_snapshot: Snapshot at the start of the validation window,
excluded from the scan
+ """
+ partition_set: dict[int, set[Record]] = {}
Review Comment:
nit: You can use a `collections.defaultdict` to avoid having to setup the
default sets.
##########
pyiceberg/table/update/snapshot.py:
##########
Review Comment:
It looks like this new method won't catch anything outside of the commit
window. Java still looks through everything (which may be wasteful).
I say we punt that to a follow-up PR just so we can unblock the release.
##########
tests/table/test_commit_retry.py:
##########
@@ -323,6 +323,79 @@ def
test_concurrent_overwrite_overwrite_raises_validation_exception(catalog: Cat
tbl2.overwrite(pa.table({"x": [40, 50, 60]}), overwrite_filter="x > 0")
[email protected](
+ ("concurrently_deleted_file", "expect_conflict", "expected_values"),
+ [
+ pytest.param("target", True, [1, 3], id="target-file"),
+ pytest.param("same-partition", False, [1, 2],
id="same-partition-file"),
+ pytest.param("different-partition", False, [2, 3],
id="different-partition-file"),
+ ],
+)
+def test_file_overwrite_validates_concurrent_file_delete(
+ catalog: Catalog,
+ concurrently_deleted_file: str,
+ expect_conflict: bool,
+ expected_values: list[int],
+) -> None:
+ """A file replacement must fail only when its target file was concurrently
deleted."""
+ import uuid
Review Comment:
nit: Since this is in the stdlib, we can probably put it on the normal
imports up top.
--
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]
