Repository navigation
Conversation
A partial delete reads each candidate file in full and writes back the surviving rows, but planned the rewrite with the delete filter, which also prunes delete files by column bounds. Rows removed by a pruned delete file came back in the rewritten file. Plan the rewrite without pruning delete files; scans are unchanged.
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 #4291.
Rationale for this change
A partial
Table.deleterewrites each data file that might hold matching rows. It reads the whole file withAlwaysTrue()and writes back the rows that don't match. The files to rewrite were planned with the delete filter, though, and that plan also leaves out delete files whose column bounds can't match the filter. For a filtered read that's correct, because the residual drops those rows anyway. For a full-file rewrite it isn't: the left-out delete files are never applied, so the rows they deleted come back in the new data file.overwrite(..., overwrite_filter=...),dynamic_partition_overwriteandupsertgo throughTransaction.delete, so they're affected too.The fix keeps the delete filter for choosing data files, but stops pruning delete files by their column bounds when planning a rewrite:
ManifestGroupPlannertakesprune_delete_files: bool = True. When it'sFalse, the metrics evaluator is applied to data entries only. Partition pruning is unchanged, since a delete file only applies to data files in its own partition.Transaction.deleteplans its rewrite withprune_delete_files=False. Its scan never has a catalog, so it always planned locally, and server-side planning isn't involved.Scans keep the default, so reads and their planning cost are unchanged.
Are these changes tested?
Yes.
test_partial_delete_keeps_earlier_deletes_outside_the_filterintests/catalog/test_catalog_behaviors.pyruns against each catalog fixture and follows case 1 from the issue:(2, 'b'), which gives it boundsk = 2;k = 1leaves that delete file out;delete("k = 1")and checks onlyk = 3is left.Without the fix the final assertion fails with
[2, 3] == [3]: the deleted row is back. PyIceberg can't write delete manifests yet, so the test's helper subclassesManifestWriterV2, as the reproduction in the issue does. Once #4002 lands, the helper can passcontent=ManifestContent.DELETESinstead.The equality-delete case from the issue isn't tested here, because PyIceberg can't read equality deletes. With this change, a partial delete over a file that has an equality delete now reaches the existing "does not yet support equality deletes" error instead of silently writing the deleted rows back.
Are there any user-facing changes?
Partial deletes, filtered overwrites and upserts no longer resurrect rows removed by earlier delete files. A partial delete on a table with equality delete files that only the column bounds used to exclude now raises the existing unsupported-equality-deletes error, rather than writing incorrect data.
Not in this PR: the issue also points out that the retry conflict check (
_filter_manifest_entriesintable/update/validate.py) prunes delete entries the same way. That's a separate code path, so I'll follow up on it separately.