Fix residual ordered comparisons for null identity partitions - #3817
Open
kevinjqliu wants to merge 2 commits into
Open
kevinjqliu wants to merge 2 commits into
kevinjqliu wants to merge 2 commits into
Conversation
kevinjqliu
marked this pull request as draft
August 21, 2026 18:14
kevinjqliu
force-pushed
the
kevinjqliu/codex-null-residual-comparisons
branch
from
August 21, 2026 18:36
85e420d to
18c8c94
Compare
kevinjqliu
changed the title
| def visit_less_than(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression: | ||
| if term.eval(self.struct) < literal.value: | ||
| value = term.eval(self.struct) | ||
| if value is None or value < literal.value: |
Collaborator
There was a problem hiding this comment.
This seems like another issue where AlwaysFalse and AlwaysTrue both evaluate to True.
I'm happy to create a small helper method that we can use in these situations
Contributor
Author
There was a problem hiding this comment.
this ones slightly different. the issue is that the term.eval(self.struct) can resolve to None, and in python
None < literal.value comparison is a type error.
kevinjqliu
force-pushed
the
kevinjqliu/codex-null-residual-comparisons
branch
11 times, most recently
from
August 22, 2026 15:43
46c8202 to
70015b5
Compare
ResidualVisitor diverged from row-level expression evaluation on null values: - visit_less_than / visit_less_than_or_equal / visit_greater_than / visit_greater_than_or_equal compared the partition value to the literal directly. A nullable identity-partitioned column with a None partition value raised a TypeError (None < literal), while _ExpressionEvaluator guards with "value is not None" and treats the row as non-matching. Add the same guard so a null partition value yields AlwaysFalse instead of crashing during scan planning (ResidualEvaluator.residual_for). - visit_not_nan returned AlwaysFalse for a None value because None is not a SupportsFloat, whereas _ExpressionEvaluator.visit_not_nan (val == val) treats null as satisfying not-NaN. Invert the check so only NaN fails not-NaN and null (and any non-float value) passes, matching row evaluation. Update the test that encoded the old NotNaN(None) -> AlwaysFalse result and add a regression test covering None partition values for all four ordering comparisons. Fixes apache#3498 (partially)
kevinjqliu
force-pushed
the
kevinjqliu/codex-null-residual-comparisons
branch
from
August 22, 2026 15:54
70015b5 to
a459a45
Compare
kevinjqliu
commented
Aug 22, 2026
|
|
||
| res_eval = residual_evaluator_of(spec=spec, expr=predicate, case_sensitive=True, schema=schema) | ||
|
|
||
| assert res_eval.residual_for(Record(None)) == AlwaysFalse() |
Contributor
Author
There was a problem hiding this comment.
resolving to false makes sense because null is not less than 1 😄
|
|
||
| assert len(tasks) == 1 | ||
| assert tasks[0].residual == EqualTo("y", 2) | ||
| assert scan.count() == 1 # Only the y == 2 row matches. |
Contributor
Author
There was a problem hiding this comment.
added an user facing test
|
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. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3498
Summary
Fix
ResidualVisitorfor<,<=,>, and>=on nullable identity partitions. Comparing a null partition value (None) with a literal raisedTypeErrorduring scan planning.A null value does not satisfy any ordered comparison with a literal, so the residual is
AlwaysFalse.Supersedes #3520. The
NotNaNfix is already in #3689.Tests
DataScan.count()regression test for<