dossett opened a new pull request, #3656: URL: https://github.com/apache/iceberg-python/pull/3656
*I used CODEX to analyze this problem and create this PR. I've reviewed the code and test and stand by them. This summary is written completely by a human (me) other than very light copy editing by an LLM.* Each manifest is able to use its own `_ExpressionEvaluator` and they are not reused across manifests or workers for the same manifest. This is a performance win over creating one for each file because the constructor performs the binding of the partition predicate to the partition scheme. My local py-spy showed an order of magnitude more time being spent constructing the evaluator than running the evaluation. ---- During local scan planning, PyIceberg evaluates each data file's partition values against a projected scan predicate. Profiling showed that manifest workers were spending most of their active time reconstructing `_ExpressionEvaluator` and rebinding the same predicate for every file, while the actual partition evaluation was comparatively cheap. The existing partition evaluator callable is cached per partition spec and shared across manifest worker threads. Because `_ExpressionEvaluator.eval()` mutates its current partition record, a single evaluator cannot safely be stored in that shared callable. This change keeps the immutable partition schema and projected expression cached per spec, but caches a factory instead of a mutable evaluator. The factory creates a separate, lazily initialized evaluator for each manifest task. Files within that manifest are processed sequentially and reuse the evaluator, while concurrent manifests never share mutable state. ## Performance The focused benchmark evaluates 1,000 files using a two-field partition spec and a 66-leaf projected predicate. Results are the mean of three in-process runs: | Files per manifest | `main` | This PR | Result | | ---: | ---: | ---: | ---: | | 1,000 | 0.855 s | 0.063 s | ~13.6x faster | | 1 | 0.847 s | 0.859 s | Effectively unchanged | The second case exercises the boundary where no evaluator reuse is possible. It shows that creating a task-local closure does not introduce a meaningful regression when every manifest contains only one file. This benchmark includes partition evaluator construction, predicate binding, and evaluation. It excludes catalog access, manifest Avro I/O and decompression, metrics evaluation, residual planning, and downstream data reads, so it is not an end-to-end query-runtime claim. ## Testing - Added coverage proving a manifest callable constructs one evaluator and reuses it across files. - Added coverage proving the immutable factory is cached once per partition spec while separate manifests receive separate evaluators. - Added coverage proving reused evaluators replace their mutable partition-record state between files. - Added focused benchmarks for both many files per manifest and one file per manifest. - Planner and expression-evaluator tests: 183 passed. - Full unit suite: 3,770 passed, 1,560 deselected. - All repository hooks pass, including Ruff, mypy, pydocstyle, codespell, and lockfile validation. There are no API or result-semantics changes. ## AI assistance Codex assisted with implementation, test scaffolding, benchmark design, validation, and PR drafting. -- 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]
