Fix/3758 partition predicate rootcause - #3782
Open
amitpoorab wants to merge 2 commits into
Open
Conversation
`upsert()` is the common way to reach the rewrite path, but `Table.delete()` with a predicate that spares part of a data file reaches it directly. Cover that entry point against a day-partitioned table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Manifest pruning for an overwrite derived its partition predicate from `Transaction._build_partition_predicate`, which compares a source column against a partition value. A data file records its partition values already transformed, so this only holds for identity transforms. The helper's other caller, `dynamic_partition_overwrite`, rejects non-identity transforms before reaching it; the pruning path added in apache#3011 has no such restriction. Projecting that predicate back onto the spec then transforms the value a second time, so the evaluator misses the manifest holding the file being replaced. `_existing_manifests` carries that manifest over whole beside the rewritten file, leaving the superseded rows visible, and `_deleted_entries` records nothing. Bucket partitioning fails earlier still, raising TypeError when a bucket ordinal cannot bind to a string column. `Table.delete()` reaches the same path, so `upsert()` is the common way to hit this rather than the only one. Build the filter over the partition fields directly, in `_OverwriteFiles`. That is the domain a manifest evaluator binds against, so no projection is needed and the transform is never reapplied. `_OverwriteFiles` is also the only consumer: `delete_by_predicate` is called solely on `_DeleteFiles`, so the predicate the base class threaded through never applied to anything else. Keeping it here leaves `_predicate` meaning what it did before apache#3011, a row-level filter over source columns, and takes the side effect back out of `_manifests()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Rationale for this change
On temporal-partitioned tables,
upsert()was silently corrupting data by leaving replaced rows in place and duplicating untouched rows.Table.delete()on partial rewrites had the same issue.Root cause: Manifest pruning derived partition predicates from source columns + partition values, then re-applied transforms during projection. This double-transformation caused false negatives in manifest evaluation, missing the manifest holding the file being replaced.
The fix: Build partition-space predicates directly using partition field names (e.g.,
ts_dayinstead ofts), avoiding re-transformation. This works for ALL transform types (identity, temporal day/month/year/hour, bucket, truncate) — not just identity.Credits: Commits cherry-picked from @paulcaron16k's investigation branch. They implemented the fix; I diagnosed the root cause and brought it forward based on maintainer feedback.
Are these changes tested?
Yes. Added comprehensive regression tests:
test_upsert_partial_rewrite_of_partitioned_file— Testsupsert()on 7 transform types:Are there any user-facing changes?
Yes. This fixes a data corruption bug (regression from v0.11.1) in
upsert()anddelete()operations on temporal-partitioned tables. Users with day/month/year/hour-partitioned tables will now get correct results instead of silent data loss.