Skip to content

Commit baa3fe6

Browse files
tanmayrauthkevinjqliu
authored andcommitted
Fix ResidualVisitor null handling for comparisons and not-NaN
ResidualVisitor diverged from row-level expression evaluation on null values: - visit_less_than / visit_less_than_or_equal / visit_greater_than / visit_greater_than_or_equal compared the partition value to the literal directly. A nullable identity-partitioned column with a None partition value raised a TypeError (None < literal), while _ExpressionEvaluator guards with "value is not None" and treats the row as non-matching. Add the same guard so a null partition value yields AlwaysFalse instead of crashing during scan planning (ResidualEvaluator.residual_for). - visit_not_nan returned AlwaysFalse for a None value because None is not a SupportsFloat, whereas _ExpressionEvaluator.visit_not_nan (val == val) treats null as satisfying not-NaN. Invert the check so only NaN fails not-NaN and null (and any non-float value) passes, matching row evaluation. Update the test that encoded the old NotNaN(None) -> AlwaysFalse result and add a regression test covering None partition values for all four ordering comparisons. Fixes #3498 (partially)
1 parent df0e73e commit baa3fe6

2 files changed

Lines changed: 23 additions & 4 deletions

File tree

pyiceberg/expressions/visitors.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1880,25 +1880,29 @@ def visit_not_nan(self, term: BoundTerm) -> BooleanExpression:
18801880
return self.visit_true()
18811881

18821882
def visit_less_than(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
1883-
if term.eval(self.struct) < literal.value:
1883+
value = term.eval(self.struct)
1884+
if value is not None and value < literal.value:
18841885
return self.visit_true()
18851886
else:
18861887
return self.visit_false()
18871888

18881889
def visit_less_than_or_equal(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
1889-
if term.eval(self.struct) <= literal.value:
1890+
value = term.eval(self.struct)
1891+
if value is not None and value <= literal.value:
18901892
return self.visit_true()
18911893
else:
18921894
return self.visit_false()
18931895

18941896
def visit_greater_than(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
1895-
if term.eval(self.struct) > literal.value:
1897+
value = term.eval(self.struct)
1898+
if value is not None and value > literal.value:
18961899
return self.visit_true()
18971900
else:
18981901
return self.visit_false()
18991902

19001903
def visit_greater_than_or_equal(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
1901-
if term.eval(self.struct) >= literal.value:
1904+
value = term.eval(self.struct)
1905+
if value is not None and value >= literal.value:
19021906
return self.visit_true()
19031907
else:
19041908
return self.visit_false()

tests/expressions/test_residual_evaluator.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
IsNaN,
2929
IsNull,
3030
LessThan,
31+
LessThanOrEqual,
3132
NotIn,
3233
NotNaN,
3334
NotNull,
@@ -235,6 +236,20 @@ def test_is_not_nan() -> None:
235236
assert residual == AlwaysTrue()
236237

237238

239+
def test_comparison_residual_with_null_partition_value() -> None:
240+
# Regression test for https://github.com/apache/iceberg-python/issues/3498
241+
# A nullable identity-partitioned column whose partition value is None must not raise a
242+
# TypeError when compared against a literal; it should behave like row evaluation, where a
243+
# null value never satisfies an ordering predicate.
244+
schema = Schema(NestedField(50, "x", IntegerType(), required=False), NestedField(51, "hour", IntegerType()))
245+
spec = PartitionSpec(PartitionField(50, 1050, IdentityTransform(), "x_part"))
246+
247+
for predicate in (LessThan("x", 1), LessThanOrEqual("x", 1), GreaterThan("x", 1), GreaterThanOrEqual("x", 1)):
248+
res_eval = residual_evaluator_of(spec=spec, expr=predicate, case_sensitive=True, schema=schema)
249+
residual = res_eval.residual_for(Record(None))
250+
assert residual == AlwaysFalse(), f"null partition value should not match {predicate}"
251+
252+
238253
def test_not_in_timestamp() -> None:
239254
schema = Schema(NestedField(50, "ts", TimestampType()), NestedField(51, "dateint", IntegerType()))
240255

0 commit comments

Comments
 (0)