bug: fix delete_data_file on partitioned tables - #3780
Conversation
|
The test cases we had which showed the #3758 issue, in upsert and delete, all pass cleanly with this. Additional tests - no package code change - which cover the code paths not hit by the upsert test. Cherry pick into this PR if you want, or I can open an issue to contribute them. These show RED in 0.12rc1 and GREEN with this PR. |
|
|
||
| def _build_partition_predicate( | ||
| self, partition_records: set[Record], spec: PartitionSpec, schema: Schema | ||
| self, partition_records: set[Record], spec: PartitionSpec, schema: Schema, is_projected: bool = False |
There was a problem hiding this comment.
what do you think about https://github.com/apache/iceberg-python/compare/main...kevinjqliu:iceberg-python:kevinjqliu/codex-overwrite-pruning-regression-tests?expand=1?
I like that we can use _OverwriteFiles.partition_filters as the api boundary. And we can skip the new is_projected state
There was a problem hiding this comment.
I like the idea of reusing Transaction._build_partition_predicate here, but I agree the is_projected flag isn't the cleanest way to express that. Looking at the link build_partition_record_filter ends up duplicating most of the same logic.
We could have a single shared _build_partition_predicate, add two thin wrapper methods that call it with different field sets (schema-based vs. spec-based) so we dont have duplicate logic
wdyt?
There was a problem hiding this comment.
I personally like that a lot. I'm wondering if we actually need the wrapper methods and couldn't just pass in the field sets?
There was a problem hiding this comment.
oh yes, they do look similar. lets reuse it if we can. codex mentioned something about this, but i forgot. lets give it a try
There was a problem hiding this comment.
sounds good Ill make the changes will push again in a bit
12fd8eb to
f1a5464
Compare
f1a5464 to
e14a8dc
Compare
|
@kevinjqliu @rambleraptor lmk if you like this solution, I just made the caller provide the fields to the predicate building method as recommended |
QlikFrederic
left a comment
QlikFrederic
left a comment
There was a problem hiding this comment.
Thank you! Very nice fix. Tested in our local setup and fixes #3779.
|
im trying to untangle the issue. so the original issue (#3758) is fixed by https://github.com/apache/iceberg-python/pull/3320/changes#diff-23e8153e0fd497a9212215bd2067068f3b56fa071770c7ef326db3d3d03cee9bR833 context: #3758 (comment) i think this PR can still help address #3779. Taking a look to see how whether this is still sound after rebasing with main |
| from pydantic import BaseModel, ValidationError | ||
| from pytest_lazy_fixtures import lf | ||
|
|
||
| from pyiceberg.catalog import Catalog |
There was a problem hiding this comment.
Is this a stray import?
There was a problem hiding this comment.
this is used by the new test's function signature
def test_overwrite_delete_data_file_on_bucket_partition(catalog: Catalog) -> None:
There was a problem hiding this comment.
LGTM, thanks for the fix!
The original issue (#3758) has already been resolved (see #3758 (comment))
The current PR is useful to resolve #3779. I added a new regression test test_overwrite_delete_data_file_on_bucket_partition to validate
Since this PR was opened, #3320 added commit retries. For file-level overwrites, a retry may not detect that another writer deleted the file being replaced, potentially reintroducing deleted data. I’ll add file-existence validation and regression coverage in a follow-up.
kevinjqliu
changed the title
|
Thanks for the PR @gabeiglio and thank you for the reviews @paulcaron16k @amitpoorab @rambleraptor @QlikFrederic |
The branch had been sitting 8 ahead and 59 behind since 2026-08-06, and every one of the 11 files it touched had also moved on main. Merged rather than rebased: the branch is published, and rebasing would replay 8 commits against 59 rather than resolving the same content once. ## What the conflicts decided **`capabilities.py`: main wins outright, and it already knew why.** The branch's contribution was `DERIVATION_METHODS` -- match a *set* of names, because the upstream fix renamed `_build_delete_files_partition_predicate` to `_build_delete_files_partition_filters` and a single-name `hasattr` had turned that rename into "this build is unusable". Main went further and abandoned name matching altogether: ZMBNI-1109 enumerated the symbols across builds and found `_build_delete_files_partition_predicate` present on 0.12.0rc1, *which corrupts data*, and on both attempted fixes -- so no name can separate a safe build from an unsafe one. The probe is behavioural now. The branch's test went with it; main's five replace it, including one that asserts a symbol cannot override an observation. **`tests/test_compaction.py`: both sides kept.** Two additive sections at the same place, no shared test name -- main's bounded-reads work and the branch's streaming-write cases. Resolved wrong the first time: rerere handed back the conflict as two hunks and concatenating them split a function body, which `IndentationError` caught before anything else did. Redone against a single recreated hunk. **`docs/tasks.md`: stays deleted.** Main froze it as `tasks_historical.md`, so the branch's status edits have nowhere to land -- ZMBNI-1101…1106 are issues #13 -#18 now, and that is where this work gets recorded. **`pyproject.toml`: main's floor, no cap.** The branch had relaxed `>=0.11.1` to `>=0.11.0` because the sibling checkout declared 0.11.0 on a tree newer than the release; it declares 0.12.0 now, so the workaround is deleted rather than carried. Dropping `<0.12` also had to happen in the `sql` extra, which still carried it -- the dev group installs `[sql,s3,trino]`, so a cap left there keeps the resolver refusing exactly the build this branch exists to test, with nothing in the diff to say why. **`docs/roadmap.md` and the regression doc: main's frame, the branch's findings.** Taking `--ours` on roadmap.md reverted 5 commits of main's edits; redone by grafting the branch's answer to open question 1 onto main's document. ## The finding: 0.12 needs the Rust core, and nothing said so `uv lock` dropped `pyiceberg-core`, and every partitioned write then failed with `NotInstalledError` from inside PyIceberg's writer. Cause: **0.12 moved `pyiceberg-core` out of the `[pyarrow]` extra into an extra of its own**, while `transforms.pyarrow_transform` still delegates to it for all six non-identity transforms. So `pyiceberg[pyarrow]` alone cannot write a day-, month-, year-, hour-, bucket- or truncate-partitioned table on 0.12. It surfaced first as `zamboni doctor` reporting `usable: False` with "the behavioural probe could not run" -- the safe direction, and a misleading diagnosis: the probe was failing on a missing optional dependency, not on pruning. Worth knowing that the probe cannot distinguish those. `test_the_rust_core_arrives_with_the_base_install` failed, which is the test doing exactly what its docstring promised: "it fails if upstream moves the dependency out of `[pyarrow]`, rather than leaving a user to discover it by writing a table". Renamed to `test_the_rust_core_is_installed_however_it_gets_here` and split into the invariant that survives the move -- the core is importable and somebody declares it -- from the 0.11-only detail of *who*. ## Verified - `zamboni doctor` on 0.12.0: `usable: True`, and `derives delete predicate` is `observed -- an overwrite on a transformed partition kept the right rows`. The upstream fix is measured here, not taken on trust. - Full suite on 0.12.0: **525 passed, 0 failed**. - ruff, ruff format, mypy clean. Ruff caught a byte-identical duplicate of `without_code_blocks` that the auto-merge produced in `tests/test_docs.py`. Upstream state: apache/iceberg-python#3780 merged 2026-08-19 and #3758 closed with it. PyPI's newest final is still 0.11.1 and the only 0.12 artifact is `0.12.0rc1`, which predates the fix -- so the cap on main lifts when 0.12.0 publishes, not because the fix landed. #13, #14, #15, #16, #17, #18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cover the pruning path that apache#3758 broke, across the shapes that reach it: every transform family, an upsert spanning two partitions, a multi-field spec, a null partition value, and the delete entry point rather than upsert. Between them these reach the And, Or and IsNull branches of the partition filter, none of which any existing test exercises. All of them fail on 0.12.0rc1 and pass on main as of apache#3780. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 participants
Closes #3779
Related to #3758
Rationale for this change
_build_delete_files_partition_predicatecollectsdata_file.partitionrecords from deleted files. These records contain already-transformed values, but they were passed toTransaction._build_partition_predicateusing source column names. This caused the partition transform to be applied a second time.Are these changes tested?
Yes, added a regression test for upsert on a table with a non-identity partition transform.
Are there any user-facing changes?
Yes, this fixes file deletion on tables with non-identity partition transforms.