Fix issue #3758: manifest pruning for transformed partitions in upsert/delete - #3782
Open
amitpoorab wants to merge 2 commits into
Open
Fix issue #3758: manifest pruning for transformed partitions in upsert/delete#3782amitpoorab wants to merge 2 commits into
amitpoorab wants to merge 2 commits into
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>
Member
|
Could you provide a more appropriate PR title? |
amitpoorab
changed the title
Author
|
Done. Thank you |
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
3 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 partitioned tables where two rows share a partition and one is replaced (partial rewrite):
day(col),month(col),year(col),hour(col): silently corrupt data (duplicates remain)bucket(col, N): raises TypeError (Cannot convert bucket ordinal to string)Both
upsert()andTable.delete()reach the same code path.Root Cause: Domain Mismatch
Predicates exist in two domains with different semantics:
ROW-space predicates (source columns + row values):
_DeleteFilesfor delete-by-predicate filtersEqualTo(Reference("ts"), timestamp_value)PARTITION-space predicates (partition field names + transformed values):
EqualTo(Reference("ts_day"), day_ordinal_value)The bug:
_OverwriteFileswas building predicates in ROW-space (mixing source column names with partition values), then trying to convert them to PARTITION-space usinginclusive_projection(). This applied transforms to values that were already transformed, corrupting the predicate:The Fix
Build partition-space predicates directly in
_OverwriteFiles:ts_day, notts)inclusive_projection()entirely — the filter is already in partition-spaceThis ensures the predicate is built in the correct domain for the manifest evaluator, eliminating the bug for all transform types.
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.