Cache residuals with stateless evaluators#3666
Open
dossett wants to merge 3 commits 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 tests and stand by them. This summary is written completely by a human (me) other than very light copy editing by an LLM.
This PR does two important things. First, it makes the
_ResidualEvaluationobject stateless to allow for more aggressive reuse. Second, it caches the residual calculated for a given partition spec and partition values. Currently the residual is calculated on a per file basis, even though the residual isn't a function of the file. Collectively, these changes make residual evaluation safer and faster._ResidualEvaluationwas mutable because itsself.structmember was mutated when evaluating a residual. This change moves the mutability into the associated visitor object, which is created on a per file basis and then discarded. Making_ResidualEvaluationstateless therefore makes it safe to reuse concurrently.The caching is relatively straight forward. A 128 starting size was somewhat arbitrary, but the benefits of caching are significant so I would not want to start too small.
CODEX-generated summary follows:
Residual planning currently creates a mutable evaluator per data file because evaluating one partition changes evaluator state. That avoids unsafe sharing, but it repeatedly prepares equivalent evaluation state and recalculates identical residuals for files whose relevant partition values are the same.
This change makes residual evaluators safe to share by keeping partition-specific state in a short-lived visitor. Scan planning then reuses one prepared evaluator per partition spec and maintains a bounded residual cache keyed by the spec and only the partition fields that can affect the filter. This reduces repeated planning work without allowing high-cardinality, unrelated partition fields to grow or defeat the cache.
Commit structure
bca46fc3— Make residual evaluation stateless: moves partition-local state into_ResidualEvaluationVisitor, prepares the partition schema once, and adds state-isolation and concurrency tests.5676096d— Cache residuals during scan planning: reuses one evaluator per spec and adds a bounded 128-entry cache keyed by relevant partition values, with planner tests covering spec isolation, irrelevant fields, multiple transforms, and eviction.e9fc9107— Benchmark residual planning: adds a benchmark using a realistic 15-leaf predicate and both repeated and unique relevant partition values.Performance
Mean of three runs planning 2,000 files with a 15-leaf predicate:
mainThe unique-values case intentionally forces cache misses and exercises evaluator reuse independently of cache hits.
Testing
pytest tests/expressions tests/table -q— 896 passedpytest tests/benchmark/test_residual_evaluator_benchmark.py -q -s -m benchmark— 2 passedUV_NO_CONFIG=1 .venv/bin/prek run -a— all hooks passed