You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dossett (via GitHub)" <gi...@apache.org> on 2026/07/15 16:58:01 UTC
[PR] Cache residuals with stateless evaluators [iceberg-python]
dossett opened a new pull request, #3666:
URL: https://github.com/apache/iceberg-python/pull/3666
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
1. [`bca46fc3` — Make residual evaluation stateless](https://github.com/dossett/iceberg-python/commit/bca46fc3): moves partition-local state into `_ResidualEvaluationVisitor`, prepares the partition schema once, and adds state-isolation and concurrency tests.
2. [`5676096d` — Cache residuals during scan planning](https://github.com/dossett/iceberg-python/commit/5676096d): 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.
3. [`e9fc9107` — Benchmark residual planning](https://github.com/dossett/iceberg-python/commit/e9fc9107): 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:
| Relevant partition values | `main` | This PR | Improvement |
|---|---:|---:|---:|
| 7 repeated values | 1.117 s | 0.004 s | ~99.6% |
| 2,000 unique values | 1.170 s | 0.456 s | ~61.0% |
The unique-values case intentionally forces cache misses and exercises evaluator reuse independently of cache hits.
## Testing
- `pytest tests/expressions tests/table -q` — 896 passed
- `pytest tests/benchmark/test_residual_evaluator_benchmark.py -q -s -m benchmark` — 2 passed
- `UV_NO_CONFIG=1 .venv/bin/prek run -a` — all hooks passed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "dossett (via GitHub)" <gi...@apache.org>.
dossett commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3770759748
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1985,6 +1970,46 @@ def visit_unbound_predicate(self, predicate: UnboundPredicate) -> BooleanExpress
return bound
+class ResidualVisitor:
+ """Find residuals for an expression using partition values.
+
+ A residual expression is made by partially evaluating an expression using partition values.
+ For example, if a table is partitioned by day(utc_timestamp) and is read with a filter expression
+ utc_timestamp > a and utc_timestamp < b, then there are 4 possible residual expressions
+ for the partition data, d:
+
+ 1. If d > day(a) and d < day(b), the residual is always true
+ 2. If d == day(a) and d != day(b), the residual is utc_timestamp > a
+ 3. If d == day(b) and d != day(a), the residual is utc_timestamp < b
+ 4. If d == day(a) == day(b), the residual is utc_timestamp > a and utc_timestamp < b
+ """
+
+ schema: Schema
+ spec: PartitionSpec
+ case_sensitive: bool
+ expr: BooleanExpression
+ partition_schema: Schema
+
+ def __init__(self, schema: Schema, spec: PartitionSpec, case_sensitive: bool, expr: BooleanExpression) -> None:
+ self.schema = schema
+ self.spec = spec
+ self.case_sensitive = case_sensitive
+ self.expr = expr
+ self.partition_schema = Schema(*spec.partition_type(schema).fields)
+
+ def eval(self, partition_data: Record) -> BooleanExpression:
+ return visit(
+ self.expr,
+ visitor=_ResidualEvaluationVisitor(
Review Comment:
It is recreated in the case of cache misses, but the visitor is fairly lightweight. There's still a net win because the heaviest work (building the partition `StructType`) is now done once per partition spec in the now immutable `ResidualEvaluator`. That's a net savings even when there are no repeated values and no cache hits, per the performance measurement code I ran (that's in the third commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "dossett (via GitHub)" <gi...@apache.org>.
dossett commented on PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#issuecomment-6442720705
@Fokko I rebased and CI is green.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3618080069
##########
pyiceberg/table/__init__.py:
##########
@@ -117,6 +119,9 @@
ALWAYS_TRUE = AlwaysTrue()
DOWNCAST_NS_TIMESTAMP_TO_US_ON_WRITE = "downcast-ns-timestamp-to-us-on-write"
+# Retain a small working set for repeated relevant partition values without adding
+# unbounded key storage when scans contain a distinct value for every data file.
+_RESIDUAL_CACHE_MAX_SIZE = 128
Review Comment:
Can we have this as a property with this as the default? That would help with some of the monkeypatching below
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1785,7 +1785,7 @@ def _can_contain_nans(self, field_id: int) -> bool:
return (nan_count := self.nan_counts.get(field_id)) is not None and nan_count > 0
-class ResidualVisitor(BoundBooleanExpressionVisitor[BooleanExpression], ABC):
Review Comment:
We're making this class private. That may be a breaking change. I'll let a maintainer decide if we can do this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3770123341
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1985,6 +1970,46 @@ def visit_unbound_predicate(self, predicate: UnboundPredicate) -> BooleanExpress
return bound
+class ResidualVisitor:
+ """Find residuals for an expression using partition values.
+
+ A residual expression is made by partially evaluating an expression using partition values.
+ For example, if a table is partitioned by day(utc_timestamp) and is read with a filter expression
+ utc_timestamp > a and utc_timestamp < b, then there are 4 possible residual expressions
+ for the partition data, d:
+
+ 1. If d > day(a) and d < day(b), the residual is always true
+ 2. If d == day(a) and d != day(b), the residual is utc_timestamp > a
+ 3. If d == day(b) and d != day(a), the residual is utc_timestamp < b
+ 4. If d == day(a) == day(b), the residual is utc_timestamp > a and utc_timestamp < b
+ """
+
+ schema: Schema
+ spec: PartitionSpec
+ case_sensitive: bool
+ expr: BooleanExpression
+ partition_schema: Schema
+
+ def __init__(self, schema: Schema, spec: PartitionSpec, case_sensitive: bool, expr: BooleanExpression) -> None:
+ self.schema = schema
+ self.spec = spec
+ self.case_sensitive = case_sensitive
+ self.expr = expr
+ self.partition_schema = Schema(*spec.partition_type(schema).fields)
+
+ def eval(self, partition_data: Record) -> BooleanExpression:
+ return visit(
+ self.expr,
+ visitor=_ResidualEvaluationVisitor(
Review Comment:
Re-creating this evaluator seems expensive on each call 🤔
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#issuecomment-5963634120
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3915647836
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1785,7 +1785,7 @@ def _can_contain_nans(self, field_id: int) -> bool:
return (nan_count := self.nan_counts.get(field_id)) is not None and nan_count > 0
-class ResidualVisitor(BoundBooleanExpressionVisitor[BooleanExpression], ABC):
Review Comment:
The constructors are the same, but the new `ResidualVisitor` is no longer a visitor. I don't think that's very nice to our downstream consumers, who _might_ be using the visitor. Do we really need to make the split?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "JackGammack (via GitHub)" <gi...@apache.org>.
JackGammack commented on PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#issuecomment-6042189690
Commenting to keep this alive as it's a very big performance improvement for us. We see planning of >240k files reduce from 15+ minutes down to 30 seconds with this change
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3770094797
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1785,7 +1785,7 @@ def _can_contain_nans(self, field_id: int) -> bool:
return (nan_count := self.nan_counts.get(field_id)) is not None and nan_count > 0
-class ResidualVisitor(BoundBooleanExpressionVisitor[BooleanExpression], ABC):
Review Comment:
Thanks for flagging. I don't think the rename is worth breaking possible imports
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#issuecomment-6451588403
@dossett Thanks for fixing this, and thanks @rambleraptor for the review 🚀
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3770106719
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1785,7 +1785,7 @@ def _can_contain_nans(self, field_id: int) -> bool:
return (nan_count := self.nan_counts.get(field_id)) is not None and nan_count > 0
-class ResidualVisitor(BoundBooleanExpressionVisitor[BooleanExpression], ABC):
Review Comment:
I see that the new class has a very similar signature. I'm fine with that
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "dossett (via GitHub)" <gi...@apache.org>.
dossett commented on PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#issuecomment-5035606348
@rambleraptor I reworked the PR to avoid the scope change of `ResidualVisitor` thank you for calling that out. I also added a property for the cache size and correspondingly simplified the tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "dossett (via GitHub)" <gi...@apache.org>.
dossett commented on code in PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666#discussion_r3917028967
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1785,7 +1785,7 @@ def _can_contain_nans(self, field_id: int) -> bool:
return (nan_count := self.nan_counts.get(field_id)) is not None and nan_count > 0
-class ResidualVisitor(BoundBooleanExpressionVisitor[BooleanExpression], ABC):
Review Comment:
@Fokko @rambleraptor Thank you, I did not apprciate those downstream consequences. I've backed out all of the changes that could break the public contract. Good news is that the performance benefits can still be retained. I updated the PR description accordingly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Cache residuals with stateless evaluators [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #3666:
URL: https://github.com/apache/iceberg-python/pull/3666
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org