Skip to content

Commit 1fc26cd

Browse files
committed
Core: Preserve explicit delete_data_file() in _DeleteFiles._compute_deletes
Fixes #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.
1 parent 68898e5 commit 1fc26cd

2 files changed

Lines changed: 73 additions & 1 deletion

File tree

pyiceberg/table/update/snapshot.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,8 @@ def _copy_with_new_status(entry: ManifestEntry, status: ManifestEntryStatus) ->
594594
existing_manifests = []
595595
total_deleted_entries = []
596596
partial_rewrites_needed = False
597+
# Preserve files explicitly requested via delete_data_file() before resetting.
598+
explicit_deletes = set(self._deleted_data_files)
597599
self._deleted_data_files = set()
598600

599601
# Determine the snapshot to read manifests from for deletion
@@ -620,7 +622,10 @@ def _copy_with_new_status(entry: ManifestEntry, status: ManifestEntryStatus) ->
620622
if self._planned_delete_files is not None:
621623
should_delete = entry.data_file in self._planned_delete_files
622624
else:
623-
should_delete = strict_metrics_evaluator(entry.data_file) == ROWS_MUST_MATCH
625+
should_delete = (
626+
entry.data_file in explicit_deletes
627+
or strict_metrics_evaluator(entry.data_file) == ROWS_MUST_MATCH
628+
)
624629

625630
if should_delete:
626631
# Based on the metadata, it can be dropped right away
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
"""Regression tests for delete_data_file() on _DeleteFiles instances."""
18+
19+
import pyarrow as pa
20+
21+
from pyiceberg.catalog import Catalog
22+
from pyiceberg.schema import Schema
23+
from pyiceberg.types import LongType, NestedField
24+
25+
26+
def test_delete_files_with_explicit_delete_data_file(catalog: Catalog) -> None:
27+
"""Calling delete_data_file() on a _DeleteFiles instance should delete the file."""
28+
catalog.create_namespace("default")
29+
table = catalog.create_table(
30+
"default.delete_explicit",
31+
Schema(NestedField(1, "x", LongType(), required=False)),
32+
)
33+
table.append(pa.table({"x": [1, 2, 3]}))
34+
35+
data_file = next(iter(table.scan().plan_files())).file
36+
37+
with table.transaction() as tx:
38+
with tx.update_snapshot().delete() as delete_snapshot:
39+
delete_snapshot.delete_data_file(data_file)
40+
41+
reloaded = catalog.load_table("default.delete_explicit")
42+
assert reloaded.scan().to_arrow()["x"].to_pylist() == []
43+
44+
45+
def test_delete_files_explicit_file_and_predicate_combined(catalog: Catalog) -> None:
46+
"""delete_data_file() and delete_by_predicate() should both take effect."""
47+
catalog.create_namespace("default")
48+
table = catalog.create_table(
49+
"default.delete_combined",
50+
Schema(NestedField(1, "x", LongType(), required=False)),
51+
)
52+
# Write two separate files
53+
table.append(pa.table({"x": [1]}))
54+
table.append(pa.table({"x": [2]}))
55+
56+
files = [task.file for task in table.scan().plan_files()]
57+
assert len(files) == 2
58+
59+
# Delete one file explicitly, leave the other
60+
with table.transaction() as tx:
61+
with tx.update_snapshot().delete() as delete_snapshot:
62+
delete_snapshot.delete_data_file(files[0])
63+
64+
reloaded = catalog.load_table("default.delete_combined")
65+
remaining = reloaded.scan().to_arrow()["x"].to_pylist()
66+
# Only the second file's data should remain
67+
assert len(remaining) == 1

0 commit comments

Comments
 (0)