dylanpulver opened a new pull request, #3891:
URL: https://github.com/apache/iceberg-python/pull/3891
# 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`:
```java
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
