From 013d0222c60b0fe0c90cd3cc2c98dbcbf8b92c71 Mon Sep 17 00:00:00 2001 From: shanejarvie Date: Wed, 16 Sep 2026 18:01:11 -0700 Subject: [PATCH] fix(validate): report every conflicting snapshot in validation errors _added_data_files and _deleted_data_files return iterators. Both validators called any() on the iterator and then built the error message from the same, now partially consumed, iterator. any() stops at the first truthy element, so the set comprehension saw only what remained: with a single conflicting entry it produced an empty set, and with several it silently dropped the first. The result was a ValidationException that could not name the snapshot it conflicted with -- 'Added data files were found matching the filter for snapshots set()!' -- which makes a real conflict hard to diagnose. Materialise the entries once before testing them. The existing tests did not catch this because they patch the helpers with a list, which can be iterated twice. The new tests patch with an iterator, as the real helpers return, and assert every conflicting snapshot id reaches the message. --- pyiceberg/table/update/validate.py | 8 ++-- tests/table/test_validate.py | 62 ++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/pyiceberg/table/update/validate.py b/pyiceberg/table/update/validate.py index 0545182bf0..e0d8428806 100644 --- a/pyiceberg/table/update/validate.py +++ b/pyiceberg/table/update/validate.py @@ -190,8 +190,8 @@ def _validate_deleted_data_files( parent_snapshot: Ending snapshot on the branch being validated """ - conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot) - if any(conflicting_entries): + conflicting_entries = list(_deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)) + if conflicting_entries: conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries} raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!") @@ -325,8 +325,8 @@ def _validate_added_data_files( parent_snapshot: Ending snapshot on the branch being validated """ - conflicting_entries = _added_data_files(table, starting_snapshot, data_filter, None, parent_snapshot) - if any(conflicting_entries): + conflicting_entries = list(_added_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)) + if conflicting_entries: conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries if entry.snapshot_id is not None} raise ValidationException(f"Added data files were found matching the filter for snapshots {conflicting_snapshots}!") diff --git a/tests/table/test_validate.py b/tests/table/test_validate.py index a19983fd66..2ae659fdfe 100644 --- a/tests/table/test_validate.py +++ b/tests/table/test_validate.py @@ -363,6 +363,68 @@ class DummyEntry: ) +def test_validate_added_data_files_reports_every_conflicting_snapshot( + table_v2_with_extensive_snapshots_and_manifests: tuple[Table, dict[int, list[ManifestFile]]], +) -> None: + """The helpers return iterators, so the entries must survive the truthiness check. + + `any(entries)` consumes the iterator up to the first truthy element, leaving the + set comprehension that builds the error message to read what is left. With a + single conflicting entry that produced an empty set, i.e. a ValidationException + that could not name the snapshot it conflicted with. + """ + table, _ = table_v2_with_extensive_snapshots_and_manifests + oldest_snapshot = table.snapshots()[0] + newest_snapshot = cast(Snapshot, table.current_snapshot()) + + class DummyEntry: + def __init__(self, snapshot_id: int) -> None: + self.snapshot_id = snapshot_id + + for snapshot_ids in ([123], [123, 456, 789]): + with patch( + "pyiceberg.table.update.validate._added_data_files", + return_value=iter([DummyEntry(i) for i in snapshot_ids]), + ): + with pytest.raises(ValidationException) as exc_info: + _validate_added_data_files( + table=table, + starting_snapshot=newest_snapshot, + data_filter=None, + parent_snapshot=oldest_snapshot, + ) + message = str(exc_info.value) + for snapshot_id in snapshot_ids: + assert str(snapshot_id) in message, f"{snapshot_id} missing from {message!r}" + + +def test_validate_deleted_data_files_reports_every_conflicting_snapshot( + table_v2_with_extensive_snapshots_and_manifests: tuple[Table, dict[int, list[ManifestFile]]], +) -> None: + """Same iterator-consumption problem in the deleted-files validator.""" + table, _ = table_v2_with_extensive_snapshots_and_manifests + oldest_snapshot = table.snapshots()[0] + newest_snapshot = cast(Snapshot, table.current_snapshot()) + + class DummyEntry: + def __init__(self, snapshot_id: int) -> None: + self.snapshot_id = snapshot_id + + with patch( + "pyiceberg.table.update.validate._deleted_data_files", + return_value=iter([DummyEntry(123)]), + ): + with pytest.raises(ValidationException) as exc_info: + _validate_deleted_data_files( + table=table, + starting_snapshot=newest_snapshot, + data_filter=None, + parent_snapshot=oldest_snapshot, + ) + + assert "123" in str(exc_info.value) + + @pytest.mark.parametrize("operation", [Operation.APPEND, Operation.REPLACE]) def test_added_delete_files_non_conflicting_count( table_v2_with_extensive_snapshots_and_manifests: tuple[Table, dict[int, list[ManifestFile]]],