fix: delete_data_file overwrite pruning for non-identity partition specs - #3781
fix: delete_data_file overwrite pruning for non-identity partition specs#3781QlikFrederic wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes overwrite().delete_data_file(...) failures for tables using non-identity partition transforms (e.g., bucket) by disabling manifest pruning when partition predicates can’t be safely reconstructed, while preserving the pruning optimization for identity-only specs.
Changes:
- Add a non-pruning fallback (via
AlwaysTrue) when any involved partition spec contains non-identity transforms. - Add a regression test covering
delete_data_fileon a bucket-partitioned table.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/table/test_delete_data_file_manifest_pruning_bug.py | Adds regression coverage ensuring delete_data_file succeeds for bucket-partitioned tables and removes the targeted file. |
| pyiceberg/table/update/snapshot.py | Disables manifest pruning for non-identity partition specs to avoid incorrect predicate reconstruction while keeping the identity-only optimization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Comment explains that the AlwaysTrue fallback only disables the manifest-pruning optimization; deletion still happens by exact DataFile identity in _OverwriteFiles, so no rows are unexpectedly dropped. - Test now asserts on the file-path set before/after deletion (exact path removed, count drops by exactly one) instead of relying on row-count alone.
Closes #3779
Rationale for this change
overwrite().delete_data_file(...) could fail for non-identity partition transforms (for example bucket) because manifest-pruning predicate reconstruction assumes identity semantics.
Change
In snapshot.py, update _build_delete_files_partition_predicate to:
This preserves the optimization for identity-only specs while restoring working behavior for non-identity specs.
Are these changes tested?
Added test_delete_data_file_manifest_pruning_bug.py, which verifies that delete_data_file succeeds on bucket-partitioned tables and that the targeted file is removed.
Are there any user-facing changes?