Skip to content

Fix inverted null-count check in inclusive metrics NotStartsWith - #3891

Open
dylanpulver wants to merge 1 commit into
apache:mainfrom
dylanpulver:fix-inclusive-metrics-may-contain-null
Open

Fix inverted null-count check in inclusive metrics NotStartsWith#3891
dylanpulver wants to merge 1 commit into
apache:mainfrom
dylanpulver:fix-inclusive-metrics-may-contain-null

Conversation

@dylanpulver

Copy link
Copy Markdown

Rationale for this change

_InclusiveMetricsEvaluationVisitor._may_contain_null is inverted relative to Java.

visitors.py:1203 returns self.null_counts is None or (field_id in self.null_counts and self.null_counts.get(field_id) is not None). null_counts is set to EMPTY_DICT in _MetricsEvaluationVisitor.__init__, so the first disjunct is dead and the rest means "may contain null iff a count is present" — backwards on both branches.

Java, api/src/main/java/org/apache/iceberg/expressions/InclusiveMetricsEvaluator.java:100-102:

return nullCounts == null || !nullCounts.containsKey(id) || nullCounts.get(id) != 0;
null count for the field here Java
present, 0 may contain null proven no nulls
absent (unknown) proven no nulls may contain null

The only caller is visit_not_starts_with (visitors.py:1451), which uses it to guard the bounds-pruning path. The first row makes that pruning dead code. The second row prunes a file whose null count is unknown, which drops rows that do match — the row-level evaluator returns True for NotStartsWith on a NULL (visitors.py:523-524).

I have not traced whether PyIceberg's own writer emits bounds without null counts, so treat the missed-pruning half as always reachable and the row-dropping half as reachable for any spec-legal manifest but not demonstrated end to end here.

Are these changes tested?

tests/expressions/test_evaluator.py::test_string_not_starts_with is a port of Java's testStringNotStartsWith minus FILE_5, the one fixture that reaches the pruning branch. All 12 of its assertions are assert should_read; there are none of the other polarity, so a function that always answers "may contain null" cannot fail it.

Added data_file_5 (mirrors Java's FILE_5: null_value_counts={3: 0}, bounds abc..abcdefghi) and data_file_6 (same, null_value_counts=None), plus three assertions.

Full suite is 4001 passed, 3 skipped on both 58749a3 and this branch — the new assertions live inside an existing test, so the count does not move and the mutants are the evidence. Reverting the source with the tests kept fails on data_file_5 (assert not True). The naive fix self.null_counts.get(field_id, 0) != 0 gets the zero-count case right and still fails on data_file_6, which is why that second fixture is there.

make lint passes all 12 hooks. Integration tests were not run.

Are there any user-facing changes?

A NotStartsWith scan now skips data files that have a zero null count and bounds entirely inside the prefix, and stops skipping files whose null count is unknown. Fewer files read in the first case, more in the second.


Written with AI assistance (Claude Opus 4.8). The Java references above were read from apache/iceberg main, and the measurements were run against this branch.

`_InclusiveMetricsEvaluationVisitor._may_contain_null` inverted the
condition from the Java implementation. It reported "may contain null"
when a null count was present (including a count of 0) and "cannot
contain null" when the count was absent.

Both directions are wrong. A file with a proven null count of 0 was
never pruned by `NotStartsWith`, and a file whose null count is unknown
could be pruned even though its nulls satisfy the predicate, silently
dropping matching rows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dylanpulver
dylanpulver force-pushed the fix-inclusive-metrics-may-contain-null branch from 3dce8e9 to 5c9e0af Compare September 1, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant