Fix: non-identity manifest pruning - #3786
Open
QlikFrederic wants to merge 10 commits into
Open
Conversation
Build the manifest-pruning predicate against the partition struct (field.name) instead of the row schema, so it works for any partition transform, not just identity. Falls back to a plain AlwaysTrue for unpartitioned specs, and combines with existing predicate-based pruning via delete_by_predicate.
…field _OverwriteFiles deletes by exact DataFile identity, so an end-to-end delete would still pass even if pruning silently degraded to a non-discriminating fallback. This test inspects the built predicate directly to guard the partition-domain fix itself.
- 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.
…ity-manifest-pruning
- Fix stale comment in _manifests(): the call no longer touches self._predicate, only self._delete_files_partition_filters. - Extract build_field_value_predicate/build_records_predicate into pyiceberg.expressions, shared by Transaction._build_partition_predicate and _build_delete_files_partition_predicate, removing near-duplicate EqualTo/IsNull/And/Or construction. - Reset self._delete_files_partition_filters at the top of _build_delete_files_partition_predicate so it starts clean on every _manifests() pass, including retries. - Update stale test_commit_retry.py docstring: conflict detection for the CoW-rewrite path now uses the user's delete filter directly (self._predicate is no longer widened by the deleted-files partition predicate), matching Java's approach and fixing a latent inconsistency where _predicate accumulated across retries while partition_filters stayed frozen from the first attempt. This is a behavior change beyond the original bug fix and should be called out in the PR description.
- Rename tests/table/test_delete_data_file_manifest_pruning_bug.py to
test_snapshot_manifest_pruning.py, matching the repo's component-named
test file convention (no _bug suffix).
- Drop the f"..._{catalog.name}" identifier suffix: all three catalog
fixture params share name="test_catalog", so it was a no-op: isolation
already comes from the per-test tmp_path.
- Add test_delete_data_file_manifest_pruning_bucket_on_same_result_type_succeeds:
a BucketTransform over an IntegerType source column, where the pre-fix
predicate's type happened to match the source column's type. Unlike the
string-source case (a loud TypeError), this variant let the buggy
row-domain predicate bind successfully and silently prune away the
manifest containing the target file, so delete_data_file reported
success without deleting anything. Verified this fails (silently, no
exception) against the pre-fix code and passes with the current fix.
…ing.py They build predicates over Record values, not expression AST nodes, so pyiceberg/expressions/__init__.py (which only defines the expression classes themselves) was the wrong home. pyiceberg/partitioning.py already owns Record/PartitionSpec semantics and both call sites already import from it, so this is a natural fit with no new import needed and no circular-import risk (partitioning.py -> expressions is a new but one-directional edge; expressions/__init__.py does not import partitioning).
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes manifest pruning during snapshot overwrite delete_data_file() for non-identity partition transforms (e.g., BucketTransform) by rebuilding pruning predicates in the partition-field domain rather than the source-column domain. This aligns pruning with how manifest partition stats are represented and prevents both type-binding errors and silent “no-op” deletes.
Changes:
- Build per-spec manifest-pruning predicates for deleted data files using partition field names + stored partition records, and OR them into the manifest evaluator.
- Refactor record-to-expression construction into
pyiceberg.partitioning.build_records_predicateand reuse it fromTransaction._build_partition_predicate. - Add regression tests covering bucket transforms (including the “same result type” silent failure case) and adjust an unrelated test docstring to reflect current behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pyiceberg/table/update/snapshot.py |
Builds delete-file pruning predicates against partition struct fields and applies them in manifest pruning. |
pyiceberg/table/__init__.py |
Reuses the new predicate builder for transaction partition predicate construction. |
pyiceberg/partitioning.py |
Adds reusable helpers to build per-record and per-set boolean predicates from Record values. |
tests/table/test_snapshot_manifest_pruning.py |
Adds regression tests ensuring bucket-partitioned delete_data_file() works and pruning predicates reference partition fields. |
tests/table/test_commit_retry.py |
Updates test docstring to reflect that conflict detection uses the user delete filter directly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+556
to
+570
| def build_field_value_predicate(field_names: list[str], field_values: Record) -> BooleanExpression: | ||
| """Build a predicate matching a single record via per-field EqualTo/IsNull, ANDed together. | ||
|
|
||
| Args: | ||
| field_names: The name to reference for each position in field_values. | ||
| field_values: The values to match, one per field name, by position. | ||
|
|
||
| Raises: | ||
| IndexError: If field_names is empty. | ||
| """ | ||
| predicates: list[BooleanExpression] = [ | ||
| EqualTo(Reference(name), field_values[pos]) if field_values[pos] is not None else IsNull(Reference(name)) | ||
| for pos, name in enumerate(field_names) | ||
| ] | ||
| return And(*predicates) if len(predicates) > 1 else predicates[0] |
Fokko
self-requested a review
August 15, 2026 18:35
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 #3779
Rationale for this change
This is a more complete fix than #3781 : this also handles the non-identity when doing manifest pruning in overwrite().delete_data_file(...) (instead of falling back to the old way)
Are these changes tested?
Added tests.
Are there any user-facing changes?