Repository navigation
Conversation
In.__new__ and NotIn.__new__ already build a literal set to check whether the predicate can collapse to AlwaysFalse / EqualTo. SetPredicate.__init__ was then building the same set again. That meant every set predicate could allocate 2K Literal models instead of K. Reuse the set created by __new__ and return early from SetPredicate.__init__ when the instance is already initialized, following the same pattern used by And.__init__. Also resolve bound_term.ref().field.field_type once per bind() instead of once for every literal in the set comprehension.
| ) -> None: | ||
| # `In.__new__` and `NotIn.__new__` have to build the literal set to pick the predicate | ||
| # class, so they initialize the instance with it. Without this guard the set is built twice. | ||
| if hasattr(self, "literals"): |
There was a problem hiding this comment.
This feels like a code smell to me. If this particular object already has literals set, then we shouldn't be calling the constructor on it again.
There was a problem hiding this comment.
@rambleraptor Fair point ..I dropped the guard....python always runs init after new returns an instance so new now hands over the set it built and init runs once +++ looking into it also surfaced a bug on main : an In built from a generator comes out empty because new consumes the iterator before init reads it so the filter silently matches nothing... Regression tests added, same speedup
Pass the set built by __new__ directly to __init__ instead of rebuilding it ...This also fixes generator inputs: rebuilding the set could consume an already-exhausted iterator and create an empty predicate Add regression coverage for generator input
|
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. |
What changed
While profiling In / NotIn, I noticed that the literal set was being built twice.
In.new already creates the set to decide whether the predicate can collapse to AlwaysFalse / EqualTo, but SetPredicate.init rebuilt it immediately afterwards.
For 100k values, that means creating 200k Literal models instead of 100k.
I changed new to pass the set it already built to the instance, and made SetPredicate.init return early when it is already populated. This follows the same pattern already used by And.
I also moved bound_term.ref().field.field_type outside the bind() comprehension so it is resolved once instead of once per literal.
Result
On 100k values:
1k -15%
10k -20%
100k -42%
The 100k case went from 1.91s → 1.11s in my benchmark.
The gain is mainly in predicate construction. bind() still has to convert each literal because those conversions are semantically meaningful, so I would not claim this patch makes binding faster end-to-end.
Scope
This only targets the duplicated work in In / NotIn.
It does not fix the larger pruning issue from #3129: above IN_PREDICATE_LIMIT, file pruning is still disabled. That's a separate change.
Validation
Existing expression tests pass unchanged: 582 passed before and after.
ruff passes, and mypy shows the same pre-existing findings.
I also checked the constructor/deserialization paths, including duplicates, empty/single-value predicates, values=, JSON round-trips, pickling and inversion.
No intentional user-facing behavior changes.
AI disclosure: I used Claude to help profile the path and draft the change. I ran the benchmarks and reviewed the implementation myself.