perf: reuse partition evaluator within manifests#3656
Open
dossett wants to merge 1 commit into
Open
Conversation
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
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.