You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Ayoubhm07 (via GitHub)" <gi...@apache.org> on 2026/09/20 18:30:44 UTC
[PR] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Ayoubhm07 opened a new pull request, #4004:
URL: https://github.com/apache/iceberg-python/pull/4004
**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.
--
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] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Posted by "Ayoubhm07 (via GitHub)" <gi...@apache.org>.
Ayoubhm07 commented on code in PR #4004:
URL: https://github.com/apache/iceberg-python/pull/4004#discussion_r4070164887
##########
pyiceberg/expressions/__init__.py:
##########
@@ -698,6 +698,11 @@ class SetPredicate(UnboundPredicate, ABC):
def __init__(
self, term: str | UnboundTerm, literals: Iterable[Any] | Iterable[LiteralValue] | None = None, **kwargs: Any
) -> 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"):
Review 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
--
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] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Posted by "Ayoubhm07 (via GitHub)" <gi...@apache.org>.
Ayoubhm07 commented on code in PR #4004:
URL: https://github.com/apache/iceberg-python/pull/4004#discussion_r4070164887
##########
pyiceberg/expressions/__init__.py:
##########
@@ -698,6 +698,11 @@ class SetPredicate(UnboundPredicate, ABC):
def __init__(
self, term: str | UnboundTerm, literals: Iterable[Any] | Iterable[LiteralValue] | None = None, **kwargs: Any
) -> 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"):
Review Comment:
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
--
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] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4004:
URL: https://github.com/apache/iceberg-python/pull/4004#issuecomment-6355389344
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] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Posted by "Ayoubhm07 (via GitHub)" <gi...@apache.org>.
Ayoubhm07 commented on code in PR #4004:
URL: https://github.com/apache/iceberg-python/pull/4004#discussion_r4070164887
##########
pyiceberg/expressions/__init__.py:
##########
@@ -698,6 +698,11 @@ class SetPredicate(UnboundPredicate, ABC):
def __init__(
self, term: str | UnboundTerm, literals: Iterable[Any] | Iterable[LiteralValue] | None = None, **kwargs: Any
) -> 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"):
Review Comment:
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
--
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] perf(expressions): avoid building literal sets twice for In / NotIn [iceberg-python]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #4004:
URL: https://github.com/apache/iceberg-python/pull/4004#discussion_r4067821411
##########
pyiceberg/expressions/__init__.py:
##########
@@ -698,6 +698,11 @@ class SetPredicate(UnboundPredicate, ABC):
def __init__(
self, term: str | UnboundTerm, literals: Iterable[Any] | Iterable[LiteralValue] | None = None, **kwargs: Any
) -> 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"):
Review 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.
--
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