bug: fix delete_data_file on partitioned tables

[gabeiglio]

Closes #3779
Related to #3758

Rationale for this change

_build_delete_files_partition_predicate collects data_file.partition records from deleted files. These records contain already-transformed values, but they were passed to Transaction._build_partition_predicate using 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.

[paulcaron16k]

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.

https://github.com/paulcaron16k/iceberg-python/tree/test/full-branch-coverage-for-double-projection-on-partition-fields

[kevinjqliu]

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

[gabeiglio]

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?

[rambleraptor]

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?

[kevinjqliu]

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

[gabeiglio]

sounds good Ill make the changes will push again in a bit

[gabeiglio]

@kevinjqliu @rambleraptor lmk if you like this solution, I just made the caller provide the fields to the predicate building method as recommended

[QlikFrederic]

Thank you! Very nice fix. Tested in our local setup and fixes #3779.

[kevinjqliu]

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

[rambleraptor]

Is this a stray import?

[kevinjqliu]

this is used by the new test's function signature

def test_overwrite_delete_data_file_on_bucket_partition(catalog: Catalog) -> None:

[kevinjqliu]

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]

Thanks for the PR @gabeiglio and thank you for the reviews @paulcaron16k @amitpoorab @rambleraptor @QlikFrederic