Repository navigation
Keep rows where a delete filter evaluates to null - #4219
Open
Rahul-cs50 wants to merge 1 commit into
Open
Rahul-cs50 wants to merge 1 commit into
Rahul-cs50 wants to merge 1 commit into
Conversation
delete() and overwrite() rewrite a data file with the rows the delete filter does not select. The complementary filter was Not(filter) OR'd with IS NULL for each column the filter does not null-check, so a null kept rows the filter matches (p == 1 OR v == 'c' with a null v) and dropped rows it does not match, where Not(filter) is null. Coalesce the filter to false before inverting it, so exactly the rows where the filter is true are deleted, the rows scan(row_filter=filter) returns. Closes apache#4184
This was referenced Nov 3, 2026
developer-rpai
left a comment
developer-rpai
left a comment
There was a problem hiding this comment.
Nice writeup, and checking against scan(row_filter=...) is the right oracle. Wrapping in coalesce(..., False) works well with Arrow's kleene and/or.
- I think the NaN branch in _expression_to_complementary_pyarrow (pyiceberg/io/pyarrow.py around L1125) now has the same problem the null branch had.
OR is_nan(col)keeps a row whenever col is NaN, even if the filter is true through another column. Takep == 1 OR d > 0with p = 1 and d = NaN: Arrow gives false for NaN > 0 (not null), so the filter is true and scan returns the row, but the complement still keeps it.d != 5is the same, since NaN != 5 is true in Arrow. Could you add a float column with a NaN to test_delete_and_overwrite_filter_with_null and check whether the NaN terms are still needed at all? Fine as a follow-up if you'd rather keep this PR to nulls. - Minor: _NullNaNUnmentionedTermsCollector still fills null_unmentioned_bound_terms, but nothing reads it anymore. You could drop that half or leave a note.
- The updated expected repr in tests/io/test_pyarrow_visitor.py is tied to pyarrow's expression formatting. A behavior assertion would be less brittle across pyarrow versions, but that's a nit.
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 #4184
Rationale for this change
When
delete()(andoverwrite()with anoverwrite_filter) has to rewrite a data file, it keeps the rows that_expression_to_complementary_pyarrowselects. That filter wasNOT f, OR'd withc IS NULLfor every columncthatfreads without a null check. Under pyarrow's three-valued logic that goes wrong both ways when a column is null:NOT fis null wherefis null, so the row is dropped even thoughfdid not match it: x=2 in the issue (p == 2,vnull) is deleted by(p == 1 AND v IS NULL) OR (p == 2 AND v == 'c').c IS NULLkeeps a row whenevercis null, even wherefis still true:p == 1 OR v == 'c'keeps x=1 (p == 1,vnull).A row should be deleted exactly where
fis true, which are the rowsscan(row_filter=f)returns. The complement is nowNOT coalesce(f, false). The NaN handling (OR is_nan(c)for columns the filter does not NaN-check) is left as it was.Are these changes tested?
Yes.
test_delete_and_overwrite_filter_with_nullintests/catalog/test_catalog_behaviors.pyruns the issue's two filters plusv != 'c'andNOT (v == 'c')throughdelete()andoverwrite()on every local catalog, and checks that the rows left are the onesscan(row_filter=...)does not return. It fails onmainand passes with this change.test_expression_to_complementary_pyarrowis updated for the new expression. Unit tests intests/io,tests/table,tests/catalogandtests/expressions: 2658 passed (the only failures are the four Kerberos tests intest_hive.py, which need thehive-kerberosextra I did not install).ruff checkandruff format --checkare clean.Are there any user-facing changes?
delete()andoverwrite(..., overwrite_filter=...)no longer delete rows the filter does not match, or keep rows it does, when a column the filter reads is null.