ghoshp83 opened a new pull request, #4064:
URL: https://github.com/apache/iceberg-python/pull/4064
Closes #4031.
### The bug
`manifest_evaluator` builds a single `_ManifestEvalVisitor` per partition
spec and returns its bound `eval`. `_OverwriteFiles._deleted_entries` then
calls that one object from the `ExecutorFactory` thread pool, while `eval` kept
the per-manifest state on the instance:
```python
def eval(self, manifest: ManifestFile) -> bool:
if partitions := manifest.partitions:
self.partition_fields = partitions # shared by every thread
return visit(self.partition_filter, self)
```
If one thread stores manifest A's summaries and another stores manifest B's
before A reads its first predicate leaf, A is judged by B's partition bounds.
When A holds the files being deleted it is skipped,
`_validate_required_deletes` never finds them, and the overwrite fails with
`ValidationException: Missing required files to delete` even though the files
are live. The reporter measures roughly 1 in 700 overwrites on production
tables with many manifests, each one discarding the commit and its work.
The opposite direction — keeping a manifest that should have been skipped —
is harmless, because the entry comparison finds nothing in it.
### The fix
Evaluate on a shallow copy, so the bound filter stays shared and read-only
while each call gets its own summaries. This is the first of the three options
@QlikFrederic suggested, and it leaves the hot path allocating one small object
per manifest rather than rebinding the filter.
As far as I can see `_deleted_entries` is the only caller that shares an
evaluator across threads — `DataScan.plan_files`, `_existing_manifests` and
`_DeleteFiles` evaluate one manifest at a time — but the fix belongs in `eval`
either way, since the object is reachable from the factory and nothing about
its contract says single-threaded.
### The test
`test_manifest_evaluator_judges_each_manifest_by_its_own_summaries` forces
the interleaving deterministically rather than racing: it holds the thread
evaluating the in-range manifest at its first predicate leaf, after the
summaries are stored, until a second thread has evaluated an out-of-range
manifest. No sleeps, and it uses the `ThreadPoolExecutor`/`Event` idiom already
present in this test module.
It fails on `main` with `assert False` on "`manifest` holds id ==
INT_MIN_VALUE and must be read" and passes with the fix.
### Verification
Run locally against `tests/expressions/test_visitors.py`:
- new test with the fix: **1 passed**
- new test with the fix reverted: **1 failed**, on the intended assertion
- whole module, fix applied: **73 passed**
- whole module on unmodified `main`: **72 passed**
I could not run the full suite locally because `tests/conftest.py` imports
`moto`, which I could not install in this environment, so those runs used
`--noconftest`; that accounts for 19 identical fixture-lookup errors in both
the before and after runs, and it is why the counts differ by exactly the one
added test. CI will cover the rest.
--
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]
