Reject out-of-range literals when binding IN / NOT IN - #3916
Conversation
`SetPredicate.bind` kept the result of `Literal.to(field_type)` for every literal. For a value outside the field's range that result is the `AboveMax`/`BelowMin` sentinel, whose value is the type's max/min, so the bound set held a literal the user never wrote. On an `int` column, `id in (1, 2**40)` matched rows where `id` equals 2147483647, and the `not in` form dropped them. `LiteralPredicate.bind` already folds these to `AlwaysTrue`/`AlwaysFalse`, and Java's `bindInOperation` filters them out of the set; do the same here. An empty set after filtering is already folded by `BoundIn`/`BoundNotIn`. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The current set-building approach in SetPredicate.bind can de-duplicate a real boundary literal against an out-of-range sentinel and then drop it during filtering, changing semantics for inputs like [IntegerType.max, IntegerType.max + 1].
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts expression binding in pyiceberg.expressions so IN / NOT IN predicates no longer “clamp” out-of-range literals into the bound set (via AboveMax/BelowMin), aligning behavior with other predicate types and preventing incorrect matches at type boundaries.
Changes:
- Update
SetPredicate.bindto dropAboveMax/BelowMinsentinels produced byLiteral.to(field_type)when bindingIN/NOT IN. - Add evaluator-level regression tests ensuring out-of-range
IN/NOT INno longer matches/excludes the clamped boundary values. - Add bind-form assertions verifying folding behavior when the filtered set becomes empty or singleton.
File summaries
| File | Description |
|---|---|
pyiceberg/expressions/__init__.py |
Filters out-of-range literal sentinels during IN / NOT IN binding so they can’t match boundary values. |
tests/expressions/test_evaluator.py |
Adds regression tests for above-max / below-min literals in IN / NOT IN binding and evaluation. |
Review details
Suppressed comments (1)
tests/expressions/test_evaluator.py:1935
- Add the analogous boundary+out-of-range assertion for the lower bound case too (e.g.,
[IntegerType.min, IntegerType.min - 1]) to ensure binding never drops a user-providedIntegerType.minwhen an out-of-range literal is present.
below_min = IntegerType.min - 1
assert In("id", [1, below_min]).bind(schema) == EqualTo("id", 1).bind(schema)
assert NotIn("id", [1, below_min]).bind(schema) == NotEqualTo("id", 1).bind(schema)
assert In("id", [below_min]).bind(schema) == AlwaysFalse()
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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))} | ||
| ) |
| 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 |
Collecting the converted literals first let an AboveMax/BelowMin sentinel
absorb a boundary value the user did write: the sentinel's value is the
type's max/min, and Literal equality compares only the value, so
`{IntAboveMax(), LongLiteral(2147483647)}` has one element. Filtering after
that dropped both, turning `id in (2147483647, 2**40)` into AlwaysFalse.
Filter inside the comprehension so a sentinel never enters the set, and add
the boundary-plus-out-of-range case to the tests.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @jackylee-ch for adding this. I believe you hit a edge case here. However, I don't like the idea of dropping these literals when binding:
For example, the following is unexpected for me:
lit = NotIn("id", [1, 2**40])
assert lit.bind(schema).as_unbound() == litWhich would fail with the change suggested by this PR.
Instead, this should be handled by the evaluators. This is also where we handle the out of bounds case of the non-set operators. I'm curious what the current behavior is when feeding this into the evaluators, maybe we can start with some tests over there.
Keep null rows when NOT IN simplifies to NotEqualTo so Arrow scans agree with the expression evaluator. Cover bounds, metrics, and int-to-long schema evolution with evaluator and file scan regression tests. Generated-by: Codex
Pushing NotEqualTo down to Arrow drops rows where the column is null. That is a pre-existing bug, not one this change introduces: a single-literal NOT IN already folded to NotEqualTo before it. It changes the result of every `!=` row filter, so it belongs in its own change rather than here. Drop the null rows from the two scan tests so they no longer depend on it. `visit_not_in` needs no change, so a NOT IN that keeps two or more literals still keeps its nulls. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
|
Rationale for this change
SetPredicate.bindkeeps theAboveMax/BelowMinsentinel thatLiteral.to()returns for a literal outside the field's range. The sentinel carries the clamped boundary value, so it matches rows at the boundary:Drop those literals while building the bound set. Filtering before the set is built matters: a sentinel is
==and hash-equal to the boundary literal it clamps to, so collecting first lets it absorb a boundary value the user did write.LiteralPredicate.bindalready folds the same sentinels for the non-set operators, andbindInOperationin the reference implementation filters them in the same place, before the set is built.The Arrow push-downs disagree about nulls today:
~isin(...)keeps them,field != valuedrops them. ANOT INthat reduces to a single literal therefore stops returning rows where the column is null, which also makes the scan agree with what adeleteusing the same filter removes. #3918 (draft) has the details.Are these changes tested?
Yes, in
tests/expressions/test_evaluator.pyandtests/io/test_pyarrow.py: the bound form,expression_evaluator, the inclusive and strict metrics evaluators, and an end-to-end scan including after a type promotion. 16 of the 20 cases fail without the change.Are there any user-facing changes?
IN/NOT INwith a literal outside the column's range no longer matches or excludes rows at the range boundary.