sjarvie opened a new pull request, #3986:
URL: https://github.com/apache/iceberg-python/pull/3986
<!--
Thanks for opening a pull request!
-->
# Rationale for this change
`_added_data_files` and `_deleted_data_files` return
`Iterator[ManifestEntry]`. Both validators call `any()` on the iterator and
then build the error message from that same, now partially consumed, iterator:
```python
conflicting_entries = _added_data_files(table, starting_snapshot,
data_filter, None, parent_snapshot)
if any(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}!")
```
`any()` stops at the first truthy element, so the set comprehension only
sees what is left:
| Conflicting entries | Snapshots reported |
|---|---|
| 1 | `set()` |
| 3 | the last 2 — the first is dropped |
The single-entry case is the common one, and it produces a
`ValidationException` that cannot name the snapshot it conflicted with:
```
ValidationException: Added data files were found matching the filter for
snapshots set()!
```
I hit this on a concurrent `Table.overwrite` with an `overwrite_filter`,
where two writers do a read-modify-write of the same key. The validation itself
is correct — there genuinely was a conflicting file — but the message gives
nothing to debug with, and "snapshots set()" reads like an internal error
rather than a real conflict.
Affects `_validate_added_data_files` and `_validate_deleted_data_files`.
Both are fixed here.
# Are these changes tested?
Yes. Two tests added to `tests/table/test_validate.py`, one per validator.
Both fail on `main` and pass with the fix:
```
FAILED
tests/table/test_validate.py::test_validate_added_data_files_reports_every_conflicting_snapshot
FAILED
tests/table/test_validate.py::test_validate_deleted_data_files_reports_every_conflicting_snapshot
AssertionError: assert '123' in 'Deleted data files were found matching the
filter for snapshots set()!'
```
With the fix, `tests/table/test_validate.py` is 18 passed.
The existing `test_validate_added_data_files_raises_on_conflict` did not
catch this because it patches the helper with a **list**, which can be iterated
twice. The new tests patch with an `iter(...)`, matching what the real helpers
return, and assert that every conflicting snapshot id reaches the message —
including the multi-entry case, which covers the silent drop as well as the
empty set.
# Are there any user-facing changes?
Only the exception message, which now names the conflicting snapshots as it
was always meant to. No behavioural change to when validation raises.
--
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]
