From 5c9e0afbc928be1d40e3c626dd4b71118b09cd8b Mon Sep 17 00:00:00 2001 From: Dylan Pulver Date: Tue, 1 Sep 2026 17:01:16 +0300 Subject: [PATCH] Fix inverted null-count check in inclusive metrics NotStartsWith `_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 --- pyiceberg/expressions/visitors.py | 4 ++- tests/expressions/test_evaluator.py | 49 ++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/pyiceberg/expressions/visitors.py b/pyiceberg/expressions/visitors.py index 51b47db991..b6bca8fdac 100644 --- a/pyiceberg/expressions/visitors.py +++ b/pyiceberg/expressions/visitors.py @@ -1200,7 +1200,9 @@ class _InclusiveMetricsEvaluationVisitor(_MetricsEvaluationVisitor): """Evaluate inclusive metrics for one data file.""" def _may_contain_null(self, field_id: int) -> bool: - return self.null_counts is None or (field_id in self.null_counts and self.null_counts.get(field_id) is not None) + # A missing null count means the count is unknown, so the column may contain nulls. + null_count = self.null_counts.get(field_id) + return null_count is None or null_count != 0 def _contains_nans_only(self, field_id: int) -> bool: if (nan_count := self.nan_counts.get(field_id)) and (value_count := self.value_counts.get(field_id)): diff --git a/tests/expressions/test_evaluator.py b/tests/expressions/test_evaluator.py index 57c06af71e..bba4156e99 100644 --- a/tests/expressions/test_evaluator.py +++ b/tests/expressions/test_evaluator.py @@ -238,6 +238,38 @@ def data_file_4() -> DataFile: ) +@pytest.fixture +def data_file_5() -> DataFile: + return DataFile.from_args( + file_path="file_5.parquet", + file_format=FileFormat.PARQUET, + partition={}, + record_count=50, + file_size_in_bytes=3, + value_counts={3: 50}, + null_value_counts={3: 0}, + nan_value_counts=None, + lower_bounds={3: to_bytes(StringType(), "abc")}, + upper_bounds={3: to_bytes(StringType(), "abcdefghi")}, + ) + + +@pytest.fixture +def data_file_6() -> DataFile: + return DataFile.from_args( + file_path="file_6.parquet", + file_format=FileFormat.PARQUET, + partition={}, + record_count=50, + file_size_in_bytes=3, + value_counts={3: 50}, + null_value_counts=None, + nan_value_counts=None, + lower_bounds={3: to_bytes(StringType(), "abc")}, + upper_bounds={3: to_bytes(StringType(), "abcdefghi")}, + ) + + def test_all_null(schema_data_file: Schema, data_file: DataFile) -> None: should_read = _InclusiveMetricsEvaluator(schema_data_file, NotNull("all_nulls")).eval(data_file) assert not should_read, "Should skip: no non-null value in all null column" @@ -1005,7 +1037,13 @@ def test_strict_metrics_evaluator_uses_empty_byte_bounds() -> None: def test_string_not_starts_with( - schema_data_file: Schema, data_file: DataFile, data_file_2: DataFile, data_file_3: DataFile, data_file_4: DataFile + schema_data_file: Schema, + data_file: DataFile, + data_file_2: DataFile, + data_file_3: DataFile, + data_file_4: DataFile, + data_file_5: DataFile, + data_file_6: DataFile, ) -> None: should_read = _InclusiveMetricsEvaluator(schema_data_file, NotStartsWith("required", "a")).eval(data_file) assert should_read, "Should read: no stats" @@ -1045,6 +1083,15 @@ def test_string_not_starts_with( # should_read = _InclusiveMetricsEvaluator(schema_data_file, NotStartsWith("required", above_max)).eval(data_file_4) # assert should_read, "Should not read: range doesn't match" + should_read = _InclusiveMetricsEvaluator(schema_data_file, NotStartsWith("required", "abc")).eval(data_file_5) + assert not should_read, "Should not read: no nulls and all strings start with the prefix" + + should_read = _InclusiveMetricsEvaluator(schema_data_file, NotStartsWith("required", "abcd")).eval(data_file_5) + assert should_read, "Should read: lower bound is shorter than the prefix" + + should_read = _InclusiveMetricsEvaluator(schema_data_file, NotStartsWith("required", "abc")).eval(data_file_6) + assert should_read, "Should read: null count is unknown, so the file may contain nulls that match" + @pytest.fixture def strict_data_file_schema() -> Schema: