Skip to content

Commit 6800bb4

Browse files
committed
chore: fix linter and mypy errors in replace API and tests
- Fixed positional argument type mismatch for `snapshot_properties` in [_RewriteFiles](iceberg-python/pyiceberg/table/update/snapshot.py) - Added missing `Catalog` type annotations to pytest fixtures in [test_replace.py](iceberg-python/tests/table/test_replace.py) - Added strict `is not None` assertions for `table.current_snapshot()` to satisfy mypy Optional checking - Auto-formatted tests with ruff
1 parent 19e8dee commit 6800bb4

2 files changed

Lines changed: 28 additions & 24 deletions

File tree

pyiceberg/table/update/snapshot.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -666,11 +666,12 @@ def _get_entries(manifest: ManifestFile) -> list[ManifestEntry]:
666666
else:
667667
return []
668668

669+
669670
class _RewriteFiles(_SnapshotProducer["_RewriteFiles"]):
670671
"""A snapshot producer that rewrites data files."""
671672

672673
def __init__(self, operation: Operation, transaction: Transaction, io: FileIO, snapshot_properties: dict[str, str]):
673-
super().__init__(operation, transaction, io, snapshot_properties)
674+
super().__init__(operation, transaction, io, snapshot_properties=snapshot_properties)
674675

675676
def _commit(self) -> UpdatesAndRequirements:
676677
# Only produce a commit when there is something to rewrite
@@ -806,6 +807,8 @@ def replace(self) -> _RewriteFiles:
806807
io=self._io,
807808
snapshot_properties=self._snapshot_properties,
808809
)
810+
811+
809812
class _ManifestMergeManager(Generic[U]):
810813
_target_size_bytes: int
811814
_min_count_to_merge: int

tests/table/test_replace.py

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,18 @@
1-
import uuid
2-
import pytest
1+
from pyiceberg.catalog import Catalog
32
from pyiceberg.manifest import DataFile, DataFileContent, FileFormat
4-
from pyiceberg.table.snapshots import Operation
5-
from pyiceberg.partitioning import PartitionSpec
63
from pyiceberg.schema import Schema
4+
from pyiceberg.table.snapshots import Operation
75
from pyiceberg.typedef import Record
86

9-
def test_replace_api(catalog):
7+
8+
def test_replace_api(catalog: Catalog) -> None:
109
# Setup a basic table using the catalog fixture
1110
catalog.create_namespace("default")
1211
table = catalog.create_table(
1312
identifier="default.test_replace",
1413
schema=Schema(),
1514
)
16-
15+
1716
# Create mock DataFiles for the test
1817
file_to_delete = DataFile.from_args(
1918
file_path="s3://bucket/test/data/deleted.parquet",
@@ -24,7 +23,7 @@ def test_replace_api(catalog):
2423
content=DataFileContent.DATA,
2524
)
2625
file_to_delete.spec_id = 0
27-
26+
2827
file_to_add = DataFile.from_args(
2928
file_path="s3://bucket/test/data/added.parquet",
3029
file_format=FileFormat.PARQUET,
@@ -34,28 +33,29 @@ def test_replace_api(catalog):
3433
content=DataFileContent.DATA,
3534
)
3635
file_to_add.spec_id = 0
37-
36+
3837
# Initially append to have something to replace
3938
with table.transaction() as tx:
4039
with tx.update_snapshot().fast_append() as append_snapshot:
4140
append_snapshot.append_data_file(file_to_delete)
42-
41+
4342
# Verify initial append snapshot
4443
assert len(table.history()) == 1
4544
snapshot = table.current_snapshot()
45+
assert snapshot is not None
46+
assert snapshot.summary is not None
4647
assert snapshot.summary["operation"] == Operation.APPEND
47-
48+
4849
# Call the replace API
49-
table.replace(
50-
files_to_delete=[file_to_delete],
51-
files_to_add=[file_to_add]
52-
)
53-
50+
table.replace(files_to_delete=[file_to_delete], files_to_add=[file_to_add])
51+
5452
# Verify the replacement created a REPLACE snapshot
5553
assert len(table.history()) == 2
5654
snapshot = table.current_snapshot()
55+
assert snapshot is not None
56+
assert snapshot.summary is not None
5757
assert snapshot.summary["operation"] == Operation.REPLACE
58-
58+
5959
# Verify the correct files are added and deleted
6060
# The summary property tracks these counts
6161
assert snapshot.summary["added-data-files"] == "1"
@@ -65,28 +65,29 @@ def test_replace_api(catalog):
6565

6666
# Verify the new file exists in the new manifest
6767
manifest_files = snapshot.manifests(table.io)
68-
assert len(manifest_files) == 2 # One for ADDED, one for DELETED
69-
68+
assert len(manifest_files) == 2 # One for ADDED, one for DELETED
69+
7070
# Check that sequence numbers were handled properly natively by verifying the manifest contents
7171
entries = []
7272
for manifest in manifest_files:
7373
for entry in manifest.fetch_manifest_entry(table.io, discard_deleted=False):
7474
entries.append(entry)
75-
75+
7676
# One entry for ADDED (new file), one for DELETED (old file)
7777
assert len(entries) == 2
78-
79-
def test_replace_empty_files(catalog):
78+
79+
80+
def test_replace_empty_files(catalog: Catalog) -> None:
8081
# Setup a basic table using the catalog fixture
8182
catalog.create_namespace("default")
8283
table = catalog.create_table(
8384
identifier="default.test_replace_empty",
8485
schema=Schema(),
8586
)
86-
87+
8788
# Replacing empty lists should not throw errors, but should produce no changes.
8889
table.replace([], [])
89-
90+
9091
# History should be completely empty since no files were rewritten
9192
assert len(table.history()) == 0
9293
assert table.current_snapshot() is None

0 commit comments

Comments
 (0)