perf: reuse partition evaluator within manifests - #3656
Closed
dossett wants to merge 1 commit into
Closed
Conversation
Fokko
pushed a commit
that referenced
this pull request
Jul 27, 2026
*I used CODEX to analyze this problem and create this PR. I've reviewed the code and tests and stand by them. This summary is written completely by a human (me) other than very light copy editing by an LLM.* Currently, because `_ExpressionEvaluator` can't be safely shared one is created per file. That's expensive relative to actually evaluating the expression. This change makes `_ExpressionEvaluator` shareable by moving the mutable parts into a new `_ExpressionEvaluationVisitor` which stays local to the per-file work of evaluation. These benchmarks are showing improvements in sub-second times, but in the production workloads I tested this on it did translate to real wall clock improvement. But irrespective of performance gains the new `_ExpressionEvaluator` is safer since nothing was preventing its accidental concurrent reuse in the future. CODEX-generated summary follows: ------- Partition pruning currently binds the same projected expression for every data file because `_ExpressionEvaluator` stores the current record as mutable instance state. That repeated preparation is expensive, but sharing the existing evaluator across workers would allow concurrent calls to mix records. This separates preparation from per-record evaluation. `_ExpressionEvaluator` binds the expression once, while every call creates a private `_ExpressionEvaluationVisitor` containing that call's record. Manifest planning can therefore prepare one evaluator per partition spec and safely share it across manifests and workers. This provides the construction-reuse benefit targeted by #3656 without depending on files within a manifest remaining sequential, and it also benefits one-file manifests. ## Summary - Split the prepared expression evaluator from the call-local mutable visitor. - Reuse one prepared partition evaluator per partition spec during manifest planning. - Add deterministic concurrent-use, prepared-state, and planner-sharing coverage. - Add a benchmark using a realistic 15-leaf predicate across dense and one-file manifests. ## Performance I compared this branch with `main` using the partition-evaluator workload in this PR. It evaluates 1,000 files with two identity partition fields and a 15-leaf predicate modeling five `(event_day range AND region_id)` branches. Timings are medians from seven samples of five iterations. | Manifest layout | `main` | This PR | Speedup | |---|---:|---:|---:| | 1 manifest × 1,000 files | 187.928 ms | 14.413 ms | 13.04× | | 1,000 manifests × 1 file | 188.000 ms | 14.584 ms | 12.89× | The improvement comes from binding the projected partition expression once per partition spec instead of once per file. This is an isolated partition-pruning benchmark rather than an end-to-end scan-planning measurement. ## Testing - `pytest tests/expressions/test_visitors.py tests/table/test_partition_evaluator_planning.py tests/table/test_init.py` (200 passed) - `pytest tests/benchmark/test_partition_evaluator_benchmark.py -m benchmark` (2 passed)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
_ExpressionEvaluatorand 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
_ExpressionEvaluatorand 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:
mainThe 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
There are no API or result-semantics changes.
AI assistance
Codex assisted with implementation, test scaffolding, benchmark design, validation, and PR drafting.