Visitor and evaluator edge cases can over-prune files or mishandle nulls

[kevinjqliu]

Several visitor/evaluator edge cases appear unsafe or inconsistent:

1. _StrictMetricsEvaluator.visit_not_equal / visit_not_in return ROWS_MUST_MATCH when a file can contain nulls or NaNs. Example stats with [null, 5] or [NaN, 5.0] and lower/upper bounds both 5 return true for NotEqualTo("x", 5) / NotIn("x", {5}), even though one row does not match. This can incorrectly mark whole files deleted.

2. _StrictMetricsEvaluator.eval returns ROWS_MUST_MATCH for record_count <= 0. record_count=0 is vacuously true, but record_count=-1 is unknown per the local comment; even AlwaysFalse() returns true.

3. ResidualVisitor comparison methods directly compare partition values to literals. A nullable identity partition value of None with LessThan("x", 1) raises TypeError, while row evaluation returns false.

4. ResidualVisitor.visit_not_nan(None) returns AlwaysFalse, while expression evaluation treats NotNaN(None) as true. Existing tests encode both behaviors, so the semantics are inconsistent.

Validated against the current tree; examples use stats/partition shapes already supported by the repo tests.

[tanmayrauth]

I went through visitors.py and the call sites, and three of the four hold up. The big one is 1: visit_not_equal and visit_not_in short-circuit on _can_contain_nulls/_can_contain_nans and return ROWS_MUST_MATCH without ever checking the bounds, where every other strict method (and Java) uses the contains*_only variants. Since the strict evaluator drives _DeleteFiles (snapshot.py:460), this isn't just over-pruning — a delete(NotEqualTo("x", 5)) against a file with stats [null, 5] would drop the whole file and take the legitimate 5 row with it, so it's effectively silent data loss. 3 and 4 are real too: the ResidualVisitor comparisons hit None < literal and raise TypeError where the row evaluator safely guards for None, and visit_not_nan(None) returns AlwaysFalse while the row evaluator treats it as true — both reachable on the scan path via residual_for. The only one I'd push back on is 2; the record_count <= 0 behavior matches upstream Java and looks intentional, though the comment could be clearer about the -1 case.

If nobody's already on this, I'd like to take it, starting with 1 since it's the data-loss path, with a regression test asserting the 5 row survives the delete, then a follow-up for the ResidualVisitor null handling.

[kevinjqliu]

Issue #3498 evaluation

Validated on PyIceberg origin/main at 7fff821199bfc111c9b8b1667b80c846d260c6c5, compared with Apache Iceberg Java main at 0b30919372df34afb632f037df88c05cdba0b134.

1. Strict metrics NotEqualTo / NotIn can falsely prove all rows match

Status: Valid, high severity, Java-parity bug.

PyIceberg returns ROWS_MUST_MATCH when a column merely can contain nulls or NaNs:

• PyIceberg visit_not_equal:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 1667 to 1695 in 7fff821

  	def visit_not_equal(self, term: BoundTerm, literal: LiteralValue) -> bool:
  	# Rows must match when X < Min or Max < X because it is not in the range
  	field_id = term.ref().field.field_id
  	if self._can_contain_nulls(field_id) or self._can_contain_nans(field_id):
  	return ROWS_MUST_MATCH
  	field = self._get_field(field_id)
  	lower_bytes = self.lower_bounds.get(field_id)
  	if lower_bytes is not None:
  	lower = _from_byte_buffer(field.field_type, lower_bytes)
  	if self._is_nan(lower):
  	# NaN indicates unreliable bounds.
  	# See the _StrictMetricsEvaluator docs for more.
  	return ROWS_MIGHT_NOT_MATCH
  	if lower > literal.value:
  	return ROWS_MUST_MATCH
  	upper_bytes = self.upper_bounds.get(field_id)
  	if upper_bytes is not None:
  	upper = _from_byte_buffer(field.field_type, upper_bytes)
  	if upper < literal.value:
  	return ROWS_MUST_MATCH
  	return ROWS_MIGHT_NOT_MATCH

• PyIceberg visit_not_in:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 1728 to 1762 in 7fff821

  	def visit_not_in(self, term: BoundTerm, literals: set[L]) -> bool:
  	field_id = term.ref().field.field_id
  	if self._can_contain_nulls(field_id) or self._can_contain_nans(field_id):
  	return ROWS_MUST_MATCH
  	field = self._get_field(field_id)
  	lower_bytes = self.lower_bounds.get(field_id)
  	if lower_bytes is not None:
  	lower = _from_byte_buffer(field.field_type, lower_bytes)
  	if self._is_nan(lower):
  	# NaN indicates unreliable bounds.
  	# See the StrictMetricsEvaluator docs for more.
  	return ROWS_MIGHT_NOT_MATCH
  	literals = {val for val in literals if lower <= val}
  	if len(literals) == 0:
  	return ROWS_MUST_MATCH
  	upper_bytes = self.upper_bounds.get(field_id)
  	if upper_bytes is not None:
  	upper = _from_byte_buffer(field.field_type, upper_bytes)
  	literals = {val for val in literals if upper >= val}
  	if len(literals) == 0:
  	return ROWS_MUST_MATCH
  	return ROWS_MIGHT_NOT_MATCH
  	def visit_starts_with(self, term: BoundTerm, literal: LiteralValue) -> bool:
  	return ROWS_MIGHT_NOT_MATCH

That is unsafe. A file with values [null, 5] and bounds [5, 5] is not guaranteed to match NotEqualTo("x", 5) or NotIn("x", {5}), because the 5 row does not match. Same idea for [NaN, 5.0].

Impact is serious because delete planning drops whole data files when strict metrics returns ROWS_MUST_MATCH:

• PyIceberg delete planning builds the strict evaluator:

  iceberg-python/pyiceberg/table/update/snapshot.py

  Lines 432 to 437 in 7fff821

  	manifest_evaluators: dict[int, Callable[[ManifestFile], bool]] = KeyDefaultDict(self._build_manifest_evaluator)
  	strict_metrics_evaluator = _StrictMetricsEvaluator(schema, self._predicate, case_sensitive=self._case_sensitive).eval
  	inclusive_metrics_evaluator = _InclusiveMetricsEvaluator(
  	schema, self._predicate, case_sensitive=self._case_sensitive
  	).eval

• PyIceberg delete planning marks the file deleted on ROWS_MUST_MATCH:

  iceberg-python/pyiceberg/table/update/snapshot.py

  Lines 459 to 463 in 7fff821

  	for entry in manifest_file.fetch_manifest_entry(io=self._io, discard_deleted=True):
  	if strict_metrics_evaluator(entry.data_file) == ROWS_MUST_MATCH:
  	# Based on the metadata, it can be dropped right away
  	deleted_entries.append(_copy_with_new_status(entry, ManifestEntryStatus.DELETED))
  	self._deleted_data_files.add(entry.data_file)

Java only short-circuits when the column contains only nulls or only NaNs:

• Java StrictMetricsEvaluator.notEq: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/StrictMetricsEvaluator.java#L341-L375
• Java StrictMetricsEvaluator.notIn: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/StrictMetricsEvaluator.java#L418-L462

Suggested fix: in visit_not_equal and visit_not_in, replace _can_contain_nulls/_can_contain_nans with _contains_nulls_only/_contains_nans_only, then continue to use bounds checks for mixed-value cases.

Suggested tests:

• NotEqualTo("x", 5) with value count 2, null count 1, bounds 5..5 returns ROWS_MIGHT_NOT_MATCH
• NotIn("x", {5}) with the same stats returns ROWS_MIGHT_NOT_MATCH
• equivalent mixed NaN stats for float/double
• all-null and all-NaN cases still return ROWS_MUST_MATCH

2. Strict metrics returns true for record_count <= 0

Status: Reproduces, but matches Java; likely not an implementation bug.

PyIceberg returns ROWS_MUST_MATCH for record_count <= 0 before visiting the expression:

• PyIceberg _StrictMetricsEvaluator.eval:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 1503 to 1516 in 7fff821

  	def eval(self, file: DataFile) -> bool:
  	"""Test whether all records within the file match the expression.
  	Args:
  	file: A data file
  	Returns: false if the file may contain any row that doesn't match
  	the expression, true otherwise.
  	"""
  	if file.record_count <= 0:
  	# Older version don't correctly implement record count from avro file and thus
  	# set record count -1 when importing avro tables to iceberg tables. This should
  	# be updated once we implemented and set correct record count.
  	return ROWS_MUST_MATCH

This means even AlwaysFalse() returns true when record_count=-1.

Java does the same:

• Java strict eval record count check: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/StrictMetricsEvaluator.java#L88-L100

Suggested action: do not change this as part of a Java-parity fix. If desired, clarify the comment or open a separate design discussion.

3. Residual comparisons raise on nullable identity partition values

Status: Valid PyIceberg runtime bug; Java does not throw for comparable literals.

PyIceberg residual comparison methods directly compare partition values to literals:

• PyIceberg residual comparison methods:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 1852 to 1874 in 7fff821

  	def visit_less_than(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
  	if term.eval(self.struct) < literal.value:
  	return self.visit_true()
  	else:
  	return self.visit_false()
  	def visit_less_than_or_equal(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
  	if term.eval(self.struct) <= literal.value:
  	return self.visit_true()
  	else:
  	return self.visit_false()
  	def visit_greater_than(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
  	if term.eval(self.struct) > literal.value:
  	return self.visit_true()
  	else:
  	return self.visit_false()
  	def visit_greater_than_or_equal(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
  	if term.eval(self.struct) >= literal.value:
  	return self.visit_true()
  	else:
  	return self.visit_false()

For a nullable identity partition value None, LessThan("x", 1) raises:

TypeError: '<' not supported between instances of 'NoneType' and 'int'

Java residual evaluation uses literal comparators, and comparable literals use nullsFirst, so comparable null partition values do not raise:

• Java residual comparisons: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/ResidualEvaluator.java#L166-L198
• Java literal comparator: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L160-L173

Suggested fix: centralize comparison in ResidualVisitor through a null-aware comparator matching Iceberg literal ordering. Be careful: Java’s comparator behavior is not the same as PyIceberg row evaluation’s None guard. The goal for residuals should be Java parity and no runtime exception.

Suggested tests:

• nullable identity partition with None for <, <=, >, >=
• expected behavior should match Java null ordering rather than raising

4. Residual NotNaN(None) disagrees with row evaluation and Java

Status: Valid, Java-parity bug.

PyIceberg residual visit_not_nan returns false unless the value is float-like and not NaN:

• PyIceberg residual visit_not_nan:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 1845 to 1850 in 7fff821

  	def visit_not_nan(self, term: BoundTerm) -> BooleanExpression:
  	val = term.eval(self.struct)
  	if isinstance(val, SupportsFloat) and not math.isnan(val):
  	return self.visit_true()
  	else:
  	return self.visit_false()

For floating identity partitions, NotNaN(None) returns AlwaysFalse(). But PyIceberg row evaluation treats None == None as true for NotNaN(None):

• PyIceberg row evaluator visit_not_nan:

  iceberg-python/pyiceberg/expressions/visitors.py

  Lines 478 to 480 in 7fff821

  	def visit_not_nan(self, term: BoundTerm) -> bool:
  	val = term.eval(self.struct)
  	return val == val

Java also treats NotNaN(null) as true because NaNUtil.isNaN(null) is false:

• Java NaNUtil: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/util/NaNUtil.java#L25-L37
• Java residual notNaN: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/ResidualEvaluator.java#L155-L163
• Java row evaluator notNaN: https://github.com/apache/iceberg/blob/0b30919372df34afb632f037df88c05cdba0b134/api/src/main/java/org/apache/iceberg/expressions/Evaluator.java#L95-L102

Suggested fix: update residual visit_not_nan so only actual NaN values return AlwaysFalse; None and non-NaN values should return AlwaysTrue.

Suggested tests:

• update existing tests/expressions/test_residual_evaluator.py expectations for NotNaN(None)
• assert floating identity partition residual NotNaN(None) returns AlwaysTrue()

[kevinjqliu]

These are mostly resolved.

1. Strict NotEqualTo / NotIn pruning — Fixed in Fix strict NotEqualTo/NotIn pruning with partial nulls or NaNs #3521, with additional regression coverage in Add strict NotEqualTo/NotIn null and NaN tests #3547. Both are merged.
2. Strict metrics with record_count <= 0 — Addressed by #3831, currently open. It preserves zero as empty and treats negative legacy/unknown counts conservatively.
3. Residual ordered comparisons with null partitions — Addressed by Fix residual ordered comparisons for null identity partitions #3817, currently open. It covers <, <=, >, and >=, including a public scan regression test.
4. Residual NotNaN(None) semantics — Fixed by Fix residual NotNaN for null partition values #3689, which is merged.

Once #3817 and #3831 are merged, all actionable items will be resolved.