Fix NaN upper-bound check in strict metrics NotIn - #3921
Conversation
A NaN upper bound emptied the literal set through the `upper >= val` filter (False for every val), producing a false ROWS_MUST_MATCH. This can drop data files whose rows only partially match the delete filter in Table.delete(). Mirrors the existing NaN lower-bound guard.
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, targeted, and backed by a regression test that fails on main and passes with the fix.
Pull request overview
Fixes a correctness bug in PyIceberg’s strict metrics evaluator where a NaN upper bound for float/double columns could incorrectly yield ROWS_MUST_MATCH for NotIn(...), which can lead to incorrect pruning and unsafe deletes in Table.delete(...).
Changes:
- Add an explicit NaN upper-bound guard in
_StrictMetricsEvaluationVisitor.visit_not_into treat bounds as unreliable and returnROWS_MIGHT_NOT_MATCH. - Add a regression test covering
NotInwith NaN upper bounds forFloatTypeandDoubleType.
File summaries
| File | Description |
|---|---|
| pyiceberg/expressions/visitors.py | Prevents NotIn strict-eval from concluding ROWS_MUST_MATCH when the upper bound is NaN (bounds become unreliable). |
| tests/expressions/test_evaluator.py | Adds a regression test for strict NotIn evaluation with a NaN upper bound (float/double). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Column contains {1.0, NaN}: min is 1.0, max is NaN (NaN sorts greatest). | ||
| # No NaN stats are present, but the row 1.0 is in the literal set, so the | ||
| # file cannot be proven to fully match NotIn. |
There was a problem hiding this comment.
Right, that sentence was misleading: the fixture does set nan_value_counts={1: 1}. Reworded in d32ce80 to state the actual point: the non-NaN row is in the literal set, so NotIn cannot be proven even though the NaN count is known.
Rationale for this change
_StrictMetricsEvaluationVisitor.visit_not_inreturns a falseROWS_MUST_MATCHwhen theupper bound of a float column is NaN.
NaN >= valisFalsefor every value, so theupper >= valfilter drops every literalfrom the set and the empty set is read as proof that no row can match. But a NaN upper
bound only means NaN sorts greatest in the writer's min/max: the file can still contain
non-NaN values that are in the literal set.
Reproducer (offline, no catalog):
The file describes a column
{1.0, NaN}. Evaluating the predicate on the actual rowswith
expression_evaluatorgives[False, True]: the row1.0does not matchNotIn("x", {1.0, 2.0}), soROWS_MUST_MATCHis impossible.This is not just a wrong pruning verdict.
_DeleteFiles._compute_deletes(pyiceberg/table/update/snapshot.py:612) drops a whole data file when the strict
evaluator returns
ROWS_MUST_MATCH, so aTable.delete(...)with aNotInfilter candelete rows that should be kept.
Java parity:
StrictEvalVisitor.notIn(api/src/main/java/org/apache/iceberg/expressions/StrictEvalVisitor.java:358-383)
guards the lower bound explicitly with
NaNUtil.isNaN(lower)and its comparator ordersNaN greatest, so a NaN upper bound never excludes literals there. The Python port
already mirrors the lower-bound guard (visitors.py:1774); this adds the missing
upper-bound guard with the same comment, matching the Java docs note that NaN bounds
are unreliable when the column can contain non-NaN data.
Same genre as #3891 (NaN/null-bound handling in the metrics evaluators).
Are these changes tested?
Yes.
test_strict_not_in_with_nan_upper_boundin tests/expressions/test_evaluator.py,parametrized over FloatType/DoubleType, modeled on the neighboring
test_strict_not_equal_and_not_in_with_mixed_nans_and_matching_bounds. It fails onmain and passes with the fix. Full unit suite: 4019 passed, 3 skipped.
Are there any user-facing changes?
Yes, bug fix:
Table.deletewith aNotInrow filter no longer drops data files whosefloat column has a NaN upper bound and partially-matching rows. No API changes.