kevinjqliu commented on issue #3498: URL: https://github.com/apache/iceberg-python/issues/3498#issuecomment-4764719424
## 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`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L1667-L1695 - PyIceberg `visit_not_in`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L1728-L1762 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: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/table/update/snapshot.py#L432-L437 - PyIceberg delete planning marks the file deleted on `ROWS_MUST_MATCH`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/table/update/snapshot.py#L459-L463 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`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L1503-L1516 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: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L1852-L1874 For a nullable identity partition value `None`, `LessThan("x", 1)` raises: ```text 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`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L1845-L1850 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`: https://github.com/apache/iceberg-python/blob/7fff821199bfc111c9b8b1667b80c846d260c6c5/pyiceberg/expressions/visitors.py#L478-L480 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()` -- 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]
