azwanzuharimi commented on code in PR #4032:
URL: https://github.com/apache/iceberg-python/pull/4032#discussion_r4167316798
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1388,23 +1388,26 @@ def visit_in(self, term: BoundTerm, literals: set[L])
-> bool:
if self._contains_nulls_only(field_id) or
self._contains_nans_only(field_id):
return ROWS_CANNOT_MATCH
- if len(literals) > IN_PREDICATE_LIMIT:
- # skip evaluating the predicate if the number of values is too big
- return ROWS_MIGHT_MATCH
-
if not isinstance(field.field_type, PrimitiveType):
raise ValueError(f"Expected PrimitiveType: {field.field_type}")
+ # only compare the smallest and largest values if the number of values
is too big
+ above_limit = len(literals) > IN_PREDICATE_LIMIT
+
lower_bound_bytes = self.lower_bounds.get(field_id)
if lower_bound_bytes is not None:
lower_bound = from_bytes(field.field_type, lower_bound_bytes)
if self._is_nan(lower_bound):
# NaN indicates unreliable bounds. See the
InclusiveMetricsEvaluator docs for more.
return ROWS_MIGHT_MATCH
- literals = {lit for lit in literals if lower_bound <= lit} #
type: ignore[operator]
- if len(literals) == 0:
- return ROWS_CANNOT_MATCH
+ if above_limit:
+ if max(literals) < lower_bound: # type: ignore[operator]
+ return ROWS_CANNOT_MATCH
+ else:
+ literals = {lit for lit in literals if lower_bound <= lit} #
type: ignore[operator]
+ if len(literals) == 0:
+ return ROWS_CANNOT_MATCH
Review Comment:
Agreed. The first check gives the same answer in both branches, and both
scan every value once. The branches differ only at the upper bound. The filter
path narrows the set first, so it prunes a file, when all values sit outside
for both sides. The min and max path keeps that file.
So the above limit branch adds no speed and loses pruning, now removed. Same
for the manifest visitor: its `all(...)` checks stop at the first match, so a
min and max swap is never faster. Removed too.
The change is now only the removal of the two early returns above the limit.
Both visitors run the same check for all sizes. `IN_PREDICATE_LIMIT` stays in
the module because the tests use it, despite no code reads it now.
Measured on a local table with 20 data files of 100,000 rows each. The keys
all come from one file. Planning time is near equal. The scan reads fewer files
:)
| keys | files planned before | files planned after | scan time before |
scan time after |
|---|---|---|---|---|
| 201 | 20 / 20 | 1 / 20 | 22 ms | 12 ms |
| 1,000 | 20 / 20 | 1 / 20 | 39 ms | 14 ms |
| 10,000 | 20 / 20 | 1 / 20 | 239 ms | 37 ms |
| 100,000 | 20 / 20 | 1 / 20 | 2,234 ms | 279 ms |
| keys | plan before | plan after |
|---|---|---|
| 201 | 8.4 ms | 8.9 ms |
| 10,000 | 23 ms | 25 ms |
| 100,000 | 154 ms | 171 ms |
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
