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 1 commit 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.
| for spec_id in partition_to_overwrite: | ||
| if any(not isinstance(field.transform, IdentityTransform) for field in self.spec(spec_id).fields): | ||
| self.delete_by_predicate(AlwaysTrue()) | ||
| return |
| before = table.scan().to_arrow() | ||
| existing_file = next(iter(table.scan().plan_files())).file |
| after = table.scan().to_arrow() | ||
| remaining_paths = {task.file.file_path for task in table.scan().plan_files()} | ||
|
|
||
| assert existing_file.file_path not in remaining_paths | ||
| assert after.num_rows < before.num_rows |
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?