Core: Preserve explicit delete_data_file() in _DeleteFiles - #3858
Core: Preserve explicit delete_data_file() in _DeleteFiles#3858qzyu999 wants to merge 2 commits into
Conversation
1fc26cd to
b87a47c
Compare
…eletes Fixes apache#3857 _compute_deletes resets self._deleted_data_files before scanning manifests by predicate. Files added via the inherited delete_data_file() method were silently dropped. Preserve them and include them in the should_delete check alongside predicate-matched files.
b87a47c to
9813657
Compare
| @@ -0,0 +1,257 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Can you consolidate these tests into https://github.com/apache/iceberg-python/blob/main/tests/table/test_snapshots.py?
This is where the rest of the snapshot tests live.
There was a problem hiding this comment.
Done, moved all tests into test_snapshots.py and deleted the standalone file.
| existing_manifests = [] | ||
| total_deleted_entries = [] | ||
| partial_rewrites_needed = False | ||
| # Preserve files explicitly requested via delete_data_file() before resetting. |
There was a problem hiding this comment.
This seems reasonable.
| from pyiceberg.types import LongType, NestedField | ||
|
|
||
|
|
||
| @pytest.fixture() |
There was a problem hiding this comment.
Can we reuse existing fixtures?
There was a problem hiding this comment.
Hi @rambleraptor, the single-file test now reuses overwrite_table directly. Only added one new fixture (multi_file_table) for the multi-file cases, which builds on the existing catalog + arrow_table_simple fixtures rather than defining a custom schema.
2 participants
Closes #3857
Rationale for this change
_DeleteFiles._compute_deletesresetsself._deleted_data_files = set()before scanning manifests by predicate. Files added via the inheriteddelete_data_file()method were silently dropped because the reset discards them before the manifest scan begins.This means calling
delete_data_file()on a_DeleteFilesinstance (viaupdate_snapshot().delete()) produces no error and no effect.Root cause
_compute_deletesrebuilds_deleted_data_filesfrom predicate-matched entries only. Explicit file references added before the computation were lost.Fix
Preserve the explicit set before resetting, and include those files in the
should_deletecheck alongside predicate evaluation. This is a 6-line production change.Are these changes tested?
Two new tests covering:
delete_data_file()on a_DeleteFilesinstance deletes the fileBoth pass across all 3 catalog backends (memory, sql, sql_without_rowcount). Existing snapshot and commit-retry tests (169 total) continue to pass.
Are there any user-facing changes?
delete_data_file()on a_DeleteFilesinstance now correctly deletes the specified file instead of silently doing nothing.