PyArrow: NotEqualTo push-down drops null rows (draft, see description) - #3918
Draft
jackylee-ch wants to merge 1 commit into
Draft
PyArrow: NotEqualTo push-down drops null rows (draft, see description)#3918jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
An Arrow comparison yields null for a null input, so `field != value` dropped every row where the column is null. All the other evaluators treat a null as satisfying NotEqualTo: `_ExpressionEvaluationVisitor.visit_not_equal` returns `None != value`, `_StrictMetricsEvaluationVisitor` reports ROWS_MUST_MATCH for a column proven to hold only nulls, and the Arrow NOT IN path already keeps them because `~isin(...)` is false for a null. The reference implementation agrees: `Evaluator.notEq` is `!eq`, and `eq` compares with a nulls-first comparator, so a null never equals the literal. Keep the null explicitly. NOT IN needs no change. Co-Authored-By: Claude Code <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new NotEqualTo null-preserving semantics likely require coordinated updates to _expression_to_complementary_pyarrow (via _NullNaNUnmentionedTermsCollector) to avoid incorrectly preserving null rows during rewrite-based deletes/overwrites using != filters.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a semantic mismatch in the PyArrow pushdown layer (pyiceberg/io/pyarrow.py) so that NotEqualTo (!=) retains rows where the compared column is null, aligning PyArrow filtering with PyIceberg’s in-memory and metrics evaluators.
Changes:
- Update the PyArrow expression generated for
BoundNotEqualToto explicitly include null rows (is_null(col) OR (col != literal)). - Adjust the expected string representation for the generated PyArrow
NotEqualToexpression. - Add a regression test asserting pushed-down Arrow filtering matches the in-memory evaluator for
!=with nulls.
File summaries
| File | Description |
|---|---|
pyiceberg/io/pyarrow.py |
Changes NotEqualTo pushdown to preserve null rows by explicitly OR-ing with is_null. |
tests/io/test_pyarrow.py |
Updates the NotEqualTo repr expectation and adds a regression test to ensure nulls are kept. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
921
to
+925
| def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> pc.Expression: | ||
| return pc.field(self._get_field_name(term)) != _convert_scalar(literal.value, term.ref().field.field_type) | ||
| # A null is not equal to the literal, but an Arrow comparison yields null for it and | ||
| # the row would be dropped. Keep it explicitly to match the other evaluators. | ||
| ref = pc.field(self._get_field_name(term)) | ||
| return ref.is_null(nan_is_null=False) | (ref != _convert_scalar(literal.value, term.ref().field.field_type)) |
jackylee-ch
marked this pull request as draft
September 7, 2026 09:16
jackylee-ch
changed the title
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.
Rationale for this change
Draft: the change below is not the right fix, and the table explains why. Opening it as a draft to record the finding.
Pushing
NotEqualTodown to Arrow drops rows where the column is null, while_ExpressionEvaluationVisitor.visit_not_equalreturnsNone != valueand_StrictMetricsEvaluationVisitorreportsROWS_MUST_MATCHfor a column proven to hold only nulls.Evaluator.notEqin the reference implementation is!eq, andequses a nulls-first comparator, so it agrees with the evaluators.Making the push-down agree with them is not enough.
_expression_to_complementary_pyarrowORsis_nullback into the preserve filter for every term_NullNaNUnmentionedTermsCollectortreats as null-unmentioned,NotEqualToamong them, so a scan starts matching null rows while a delete with the same filter still keeps them:NOT IN (a, b)~isin[null, c][c]NOT IN (a)!=[b, c][b, c]!= a!=[b, c][b, c]The layer is already inconsistent, but in
visit_not_in, and this change moves two agreeing cases into the disagreeing column rather than fixing the one that is broken. Completing it meansdelete/overwritewould start removing null rows for a!=filter. That is a semantics change to a destructive operation, and it needs a decision on which convention to keep: the three-valued logic the Arrow layer follows today, or null-satisfies-negation as in the evaluators.Are these changes tested?
Only the scan path, which is why this is a draft.
Are there any user-facing changes?
None proposed yet.