Copilot commented on code in PR #3916:
URL: https://github.com/apache/iceberg-python/pull/3916#discussion_r3946329205
##########
pyiceberg/expressions/__init__.py:
##########
@@ -697,8 +697,13 @@ def __init__(
def bind(self, schema: Schema, case_sensitive: bool = True) ->
BoundSetPredicate:
bound_term = self.term.bind(schema, case_sensitive)
- literal_set = self.literals
- return self.as_bound(bound_term,
{lit.to(bound_term.ref().field.field_type) for lit in literal_set}) # type:
ignore
+ field_type = bound_term.ref().field.field_type
+ # Literals outside the field's range can never match, so drop them
rather
+ # than keep the clamped AboveMax/BelowMin sentinel in the bound set
+ bound_literals = {lit.to(field_type) for lit in self.literals}
+ return self.as_bound( # type: ignore
+ bound_term, {lit for lit in bound_literals if not isinstance(lit,
(AboveMax, BelowMin))}
+ )
Review Comment:
`bound_literals = {lit.to(field_type) for lit in self.literals}` builds a
set *before* filtering out `AboveMax`/`BelowMin`. Because `Literal.__eq__`
compares only by `.value`, an out-of-range sentinel (value == max/min) can be
considered equal to a user-provided boundary literal, and whichever is inserted
first “wins” in the set. If the sentinel wins, the subsequent filter drops it
and accidentally removes a legitimate literal (e.g., `In("id",
[IntegerType.max, IntegerType.max + 1])` could lose `IntegerType.max`). Filter
out sentinels *before* inserting into the set (or build the set via a loop) to
avoid this collision.
##########
tests/expressions/test_evaluator.py:
##########
@@ -1907,3 +1910,30 @@ def test_strict_metrics_eval_bounds_after_promotion(
evaluator = _StrictMetricsEvaluator(schema, op("col", lit))
assert evaluator.eval(data_file) == expected
+
+
+def test_above_int_bounds_in() -> None:
+ schema = Schema(NestedField(1, "id", IntegerType(), required=False))
+ above_max = IntegerType.max + 1
+
+ assert In("id", [1, above_max]).bind(schema) == EqualTo("id",
1).bind(schema)
+ assert NotIn("id", [1, above_max]).bind(schema) == NotEqualTo("id",
1).bind(schema)
+ assert In("id", [above_max]).bind(schema) == AlwaysFalse()
+ assert NotIn("id", [above_max]).bind(schema) == AlwaysTrue()
+
+ # The clamped literal used to match the field's maximum
+ assert expression_evaluator(schema, In("id", [1, above_max]),
True)(Record(IntegerType.max)) is False
+ assert expression_evaluator(schema, NotIn("id", [1, above_max]),
True)(Record(IntegerType.max)) is True
Review Comment:
These tests cover the pure out-of-range case, but they don’t cover the
important edge case where the user includes the boundary value *and* an
out-of-range value (e.g., `[IntegerType.max, IntegerType.max + 1]`). That
scenario can regress if out-of-range sentinels are de-duplicated against the
real boundary literal during binding; adding an assertion here would lock in
the intended behavior.
This issue also appears on line 1931 of the same file.
--
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]
