Skip to content

Commit 96aef97

Browse files
committed
Address review: test naming, catalog-name no-op, silent-skip coverage
- Rename tests/table/test_delete_data_file_manifest_pruning_bug.py to test_snapshot_manifest_pruning.py, matching the repo's component-named test file convention (no _bug suffix). - Drop the f"..._{catalog.name}" identifier suffix: all three catalog fixture params share name="test_catalog", so it was a no-op: isolation already comes from the per-test tmp_path. - Add test_delete_data_file_manifest_pruning_bucket_on_same_result_type_succeeds: a BucketTransform over an IntegerType source column, where the pre-fix predicate's type happened to match the source column's type. Unlike the string-source case (a loud TypeError), this variant let the buggy row-domain predicate bind successfully and silently prune away the manifest containing the target file, so delete_data_file reported success without deleting anything. Verified this fails (silently, no exception) against the pre-fix code and passes with the current fix.
1 parent 6e88f9d commit 96aef97

1 file changed

Lines changed: 50 additions & 2 deletions

File tree

tests/table/test_delete_data_file_manifest_pruning_bug.py renamed to tests/table/test_snapshot_manifest_pruning.py

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ def test_delete_data_file_manifest_pruning_bucket_transform_succeeds(catalog: Ca
3333
works regardless of the partition transform.
3434
"""
3535
catalog.create_namespace_if_not_exists("default")
36-
identifier = f"default.bucket_delete_bug_{catalog.name}"
36+
identifier = "default.bucket_delete"
3737

3838
schema = Schema(
3939
NestedField(1, "tenant_id", StringType(), required=True),
@@ -95,7 +95,7 @@ def test_delete_data_file_manifest_pruning_predicate_uses_partition_field(catalo
9595
non-discriminating fallback. This test guards the pruning predicate itself.
9696
"""
9797
catalog.create_namespace_if_not_exists("default")
98-
identifier = f"default.bucket_delete_pruning_predicate_{catalog.name}"
98+
identifier = "default.bucket_delete_pruning_predicate"
9999

100100
schema = Schema(
101101
NestedField(1, "tenant_id", StringType(), required=True),
@@ -137,3 +137,51 @@ def test_delete_data_file_manifest_pruning_predicate_uses_partition_field(catalo
137137
predicate = overwrite._delete_files_partition_filters[existing_file.spec_id]
138138

139139
assert predicate == EqualTo(Reference("tenant_id_bucket"), expected_bucket_id)
140+
141+
142+
def test_delete_data_file_manifest_pruning_bucket_on_same_result_type_succeeds(catalog: Catalog) -> None:
143+
"""delete_data_file must not silently skip a manifest when the bucket id happens to share the source column's type.
144+
145+
Pre-fix, the buggy predicate compared the source column (an int) against the bucket id
146+
(also an int), so binding succeeded instead of raising. The manifest's min/max stats for
147+
that column then incorrectly ruled out the manifest containing the target file, so the
148+
whole manifest was skipped and the file was silently never deleted - no exception, no
149+
error, just a delete that quietly did nothing. A string source column can't hit this path
150+
since it would fail to bind (see the other tests here), so this needs a same-result-type
151+
source to catch a regression back to the source-column domain.
152+
"""
153+
catalog.create_namespace_if_not_exists("default")
154+
identifier = "default.bucket_delete_same_result_type"
155+
156+
schema = Schema(NestedField(1, "value", IntegerType(), required=True))
157+
spec = PartitionSpec(
158+
PartitionField(
159+
source_id=1,
160+
field_id=1000,
161+
transform=BucketTransform(8),
162+
name="value_bucket",
163+
),
164+
spec_id=0,
165+
)
166+
table = catalog.create_table(
167+
identifier=identifier,
168+
schema=schema,
169+
partition_spec=spec,
170+
)
171+
table.append(
172+
pa.Table.from_pylist(
173+
[{"value": 42}],
174+
schema=pa.schema([pa.field("value", pa.int32(), nullable=False)]),
175+
)
176+
)
177+
178+
before_paths = {task.file.file_path for task in table.scan().plan_files()}
179+
existing_file = next(iter(table.scan().plan_files())).file
180+
181+
with table.transaction() as txn:
182+
with txn.update_snapshot().overwrite() as overwrite:
183+
overwrite.delete_data_file(existing_file)
184+
185+
after_paths = {task.file.file_path for task in table.scan().plan_files()}
186+
187+
assert before_paths - after_paths == {existing_file.file_path}

0 commit comments

Comments
 (0)