refactor: remove unused expire_snapshots method and clean up transaction context in MaintenanceTable

Author: ForeverAngry

pyiceberg/table/__init__.py

@@ -1250,10 +1250,6 @@ def manage_snapshots(self) -> ManageSnapshots:
         """
         return ManageSnapshots(transaction=Transaction(self, autocommit=True))
 
-    def expire_snapshots(self) -> ExpireSnapshots:
-        """Shorthand to run expire snapshots by id or by a timestamp."""
-        return ExpireSnapshots(transaction=Transaction(self, autocommit=True))
-
     def update_statistics(self) -> UpdateStatistics:
         """
         Shorthand to run statistics management operations like add statistics and remove statistics.

pyiceberg/table/maintenance.py

@@ -17,7 +17,7 @@
 from __future__ import annotations
 
 import logging
-from typing import TYPE_CHECKING, Any, List, Optional, Set, Union
+from typing import TYPE_CHECKING, List, Optional, Set
 
 from pyiceberg.manifest import DataFile, ManifestFile
 from pyiceberg.utils.concurrent import ThreadPoolExecutor  # type: ignore[attr-defined]
@@ -104,7 +104,7 @@ def expire_snapshots_older_than(self, timestamp_ms: int) -> None:
                 snapshots_to_expire.append(snapshot.snapshot_id)
 
         if snapshots_to_expire:
-            with self.tbl.transaction() as txn:
+            with self.tbl.transaction():
                 self.expire_snapshots_by_ids(snapshots_to_expire)
 
     def expire_snapshots_older_than_with_retention(
@@ -122,7 +122,7 @@ def expire_snapshots_older_than_with_retention(
         )
 
         if snapshots_to_expire:
-            with self.tbl.transaction() as txn:
+            with self.tbl.transaction():
                 self.expire_snapshots_by_ids(snapshots_to_expire)
 
     def retain_last_n_snapshots(self, n: int) -> None:
@@ -158,7 +158,7 @@ def retain_last_n_snapshots(self, n: int) -> None:
                 snapshots_to_expire.append(snapshot.snapshot_id)
 
         if snapshots_to_expire:
-            with self.tbl.transaction() as txn:
+            with self.tbl.transaction():
                 self.expire_snapshots_by_ids(snapshots_to_expire)
 
     def _get_snapshots_to_expire_with_retention(
@@ -325,7 +325,7 @@ def deduplicate_data_files(self) -> List[DataFile]:
         """
         import os
         from collections import defaultdict
-        
+
         removed: List[DataFile] = []
 
         # Get the current snapshot
@@ -349,8 +349,8 @@ def deduplicate_data_files(self) -> List[DataFile]:
         has_duplicates = False
         files_to_remove = []
         files_to_keep = []
-        
-        for file_name, entries in file_groups.items():
+
+        for _file_name, entries in file_groups.items():
             if len(entries) > 1:
                 # Keep the first entry, remove the rest
                 files_to_keep.append(entries[0].data_file)
@@ -369,11 +369,11 @@ def deduplicate_data_files(self) -> List[DataFile]:
                     # First, explicitly delete all the duplicate files
                     for file_to_remove in files_to_remove:
                         overwrite_snapshot.delete_data_file(file_to_remove)
-                    
+
                     # Then add back only the files that should be kept
                     for file_to_keep in files_to_keep:
                         overwrite_snapshot.append_data_file(file_to_keep)
-            
+
             # Refresh the table to reflect the changes
             self.tbl = self.tbl.refresh()
 

tests/table/test_dedup_data_file_filepaths.py

@@ -14,9 +14,10 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+import os
+import uuid
 from pathlib import Path
 from typing import List, Set
-import os
 
 import pyarrow as pa
 import pyarrow.parquet as pq
@@ -26,7 +27,6 @@
 from pyiceberg.table import Table
 from pyiceberg.table.maintenance import MaintenanceTable
 from tests.catalog.test_base import InMemoryCatalog
-import uuid
 
 
 @pytest.fixture
@@ -98,7 +98,9 @@ def test_overwrite_removes_only_selected_datafile(prepopulated_table: Table, dup
     assert dupe_data_file_path.name in file_names_after, f"Expected {dupe_data_file_path.name} to remain in the table"
     assert len(file_names_after) == 1, "Expected only one unique file name to remain after deduplication"
     # All removed files should have the same file name
-    assert all(df.file_path.split("/")[-1] == dupe_data_file_path.name for df in removed_files), "All removed files should be duplicates by name"
+    assert all(df.file_path.split("/")[-1] == dupe_data_file_path.name for df in removed_files), (
+        "All removed files should be duplicates by name"
+    )
 
 
 def test_get_all_datafiles_current_snapshot(prepopulated_table: Table, dupe_data_file_path: Path) -> None:
@@ -117,13 +119,17 @@ def test_get_all_datafiles_all_snapshots(prepopulated_table: Table, dupe_data_fi
     assert dupe_data_file_path.name in file_paths
 
 
-def test_deduplicate_data_files_removes_duplicates_in_current_snapshot(prepopulated_table: Table, dupe_data_file_path: Path) -> None:
+def test_deduplicate_data_files_removes_duplicates_in_current_snapshot(
+    prepopulated_table: Table, dupe_data_file_path: Path
+) -> None:
     mt = MaintenanceTable(tbl=prepopulated_table)
 
     all_datafiles: List[DataFile] = mt._get_all_datafiles()
     file_names: List[str] = [os.path.basename(df.file_path) for df in all_datafiles]
     # There should be more than one reference before deduplication
-    assert file_names.count(dupe_data_file_path.name) > 1, f"Expected multiple references to {dupe_data_file_path.name} before deduplication"
+    assert file_names.count(dupe_data_file_path.name) > 1, (
+        f"Expected multiple references to {dupe_data_file_path.name} before deduplication"
+    )
     removed: List[DataFile] = mt.deduplicate_data_files()
 
     all_datafiles_after: List[DataFile] = mt._get_all_datafiles()