Fix residual NotNaN for null partition values - #3689
Conversation
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
| if isinstance(val, SupportsFloat) and math.isnan(val): | ||
| return self.visit_false() | ||
| else: | ||
| return self.visit_true() |
There was a problem hiding this comment.
👍
previously for None, isinstance(None, SupportsFloat) is false, so it returned AlwaysFalse().
| residual = res_eval.residual_for(Record(None)) | ||
| assert residual == AlwaysTrue() | ||
|
|
||
| residual = res_eval.residual_for(Record(float("nan"))) |
There was a problem hiding this comment.
None is now true.
NaN is now false.
8086dd8 to
bbeaee9
Compare
There was a problem hiding this comment.
Pull request overview
Fixes residual evaluation so NotNaN correctly treats null identity-partition values as not-NaN.
Changes:
- Returns
AlwaysFalseonly for actual NaN values. - Adds float and double regression coverage for null partitions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pyiceberg/expressions/visitors.py |
Corrects NotNaN residual semantics. |
tests/expressions/test_residual_evaluator.py |
Tests null values for float and double partitions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
thanks for the PR @BharatDeva |
Rationale for this change
ResidualVisitor.visit_not_nancurrently treats a partition value ofNoneasAlwaysFalse(). That is inconsistent with the normal evaluator behavior and with Iceberg Java semantics:Noneis not NaN, soNotNaN(None)should evaluate to true.This PR addresses only that residual-evaluator edge case from #3498. The stricter metrics-evaluator items discussed in the issue are already covered by other active work.
Are these changes tested?
Yes. I updated the existing residual evaluator expectations for both double and float identity partitions and ran the targeted test file in a WSL-native checkout:
Result:
Additional checks:
Both passed.
Are there any user-facing changes?
Yes. Residual evaluation for
NotNaNnow treats null partition values as not-NaN instead of false. Please add the changelog label if this behavior change should appear in release notes.