From 5b85d1ca2e54e895c23a803e655aeb09f59709d9 Mon Sep 17 00:00:00 2001 From: chinmay-bhat <12948588+chinmay-bhat@users.noreply.github.com> Date: Mon, 27 May 2024 22:55:24 +0530 Subject: [PATCH 1/5] cache manifests --- pyiceberg/catalog/__init__.py | 2 +- pyiceberg/cli/output.py | 2 +- pyiceberg/table/snapshots.py | 10 +++++++--- tests/integration/test_partitioning_key.py | 8 ++++++-- tests/integration/test_rest_manifest.py | 2 +- tests/utils/test_manifest.py | 8 ++++---- 6 files changed, 20 insertions(+), 12 deletions(-) diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py index 68583bf3c9..c2aa39a70e 100644 --- a/pyiceberg/catalog/__init__.py +++ b/pyiceberg/catalog/__init__.py @@ -835,7 +835,7 @@ def purge_table(self, identifier: Union[str, Identifier]) -> None: manifest_lists_to_delete = set() manifests_to_delete: List[ManifestFile] = [] for snapshot in metadata.snapshots: - manifests_to_delete += snapshot.manifests(io) + manifests_to_delete += snapshot.manifests(io, snapshot.manifest_list) if snapshot.manifest_list is not None: manifest_lists_to_delete.add(snapshot.manifest_list) diff --git a/pyiceberg/cli/output.py b/pyiceberg/cli/output.py index 56b544c99f..e9ba377453 100644 --- a/pyiceberg/cli/output.py +++ b/pyiceberg/cli/output.py @@ -144,7 +144,7 @@ def files(self, table: Table, history: bool) -> None: manifest_list_str = f": {snapshot.manifest_list}" if snapshot.manifest_list else "" list_tree = snapshot_tree.add(f"Snapshot {snapshot.snapshot_id}, schema {snapshot.schema_id}{manifest_list_str}") - manifest_list = snapshot.manifests(io) + manifest_list = snapshot.manifests(io, manifest_list_str) for manifest in manifest_list: manifest_tree = list_tree.add(f"Manifest: {manifest.manifest_path}") for manifest_entry in manifest.fetch_manifest_entry(io, discard_deleted=False): diff --git a/pyiceberg/table/snapshots.py b/pyiceberg/table/snapshots.py index 1ccb079922..7268e63ac1 100644 --- a/pyiceberg/table/snapshots.py +++ b/pyiceberg/table/snapshots.py @@ -19,6 +19,7 @@ import time from collections import defaultdict from enum import Enum +from functools import lru_cache from typing import TYPE_CHECKING, Any, DefaultDict, Dict, Iterable, List, Mapping, Optional from pydantic import Field, PrivateAttr, model_serializer @@ -249,9 +250,12 @@ def __str__(self) -> str: result_str = f"{operation}id={self.snapshot_id}{parent_id}{schema_id}" return result_str - def manifests(self, io: FileIO) -> List[ManifestFile]: - if self.manifest_list is not None: - file = io.new_input(self.manifest_list) + @staticmethod + @lru_cache + def manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]: + """Return the manifests for the given snapshot.""" + if manifest_list not in (None, ""): + file = io.new_input(manifest_list) return list(read_manifest_list(file)) return [] diff --git a/tests/integration/test_partitioning_key.py b/tests/integration/test_partitioning_key.py index 29f664909c..f45223a224 100644 --- a/tests/integration/test_partitioning_key.py +++ b/tests/integration/test_partitioning_key.py @@ -763,10 +763,14 @@ def test_partition_key( snapshot = iceberg_table.current_snapshot() assert snapshot spark_partition_for_justification = ( - snapshot.manifests(iceberg_table.io)[0].fetch_manifest_entry(iceberg_table.io)[0].data_file.partition + snapshot.manifests(iceberg_table.io, snapshot.manifest_list)[0] + .fetch_manifest_entry(iceberg_table.io)[0] + .data_file.partition ) spark_path_for_justification = ( - snapshot.manifests(iceberg_table.io)[0].fetch_manifest_entry(iceberg_table.io)[0].data_file.file_path + snapshot.manifests(iceberg_table.io, snapshot.manifest_list)[0] + .fetch_manifest_entry(iceberg_table.io)[0] + .data_file.file_path ) assert spark_partition_for_justification == expected_partition_record assert expected_hive_partition_path_slice in spark_path_for_justification diff --git a/tests/integration/test_rest_manifest.py b/tests/integration/test_rest_manifest.py index 82c41cfd93..8c4b2aaf64 100644 --- a/tests/integration/test_rest_manifest.py +++ b/tests/integration/test_rest_manifest.py @@ -75,7 +75,7 @@ def test_write_sample_manifest(table_test_all_types: Table) -> None: if test_snapshot is None: raise ValueError("Table has no current snapshot, check the docker environment") io = table_test_all_types.io - test_manifest_file = test_snapshot.manifests(io)[0] + test_manifest_file = test_snapshot.manifests(io, test_snapshot.manifest_list)[0] test_manifest_entries = test_manifest_file.fetch_manifest_entry(io) entry = test_manifest_entries[0] test_schema = table_test_all_types.schema() diff --git a/tests/utils/test_manifest.py b/tests/utils/test_manifest.py index ef33b16b00..859264b0d3 100644 --- a/tests/utils/test_manifest.py +++ b/tests/utils/test_manifest.py @@ -217,7 +217,7 @@ def test_read_manifest_v1(generated_manifest_file_file_v1: str) -> None: summary=Summary(Operation.APPEND), schema_id=3, ) - manifest_list = snapshot.manifests(io)[0] + manifest_list = snapshot.manifests(io, snapshot.manifest_list)[0] assert manifest_list.manifest_length == 7989 assert manifest_list.partition_spec_id == 0 @@ -267,7 +267,7 @@ def test_read_manifest_v2(generated_manifest_file_file_v2: str) -> None: summary=Summary(Operation.APPEND), schema_id=3, ) - manifest_list = snapshot.manifests(io)[0] + manifest_list = snapshot.manifests(io, manifest_list=snapshot.manifest_list)[0] assert manifest_list.manifest_length == 7989 assert manifest_list.partition_spec_id == 0 @@ -336,7 +336,7 @@ def test_write_manifest( summary=Summary(Operation.APPEND), schema_id=3, ) - demo_manifest_file = snapshot.manifests(io)[0] + demo_manifest_file = snapshot.manifests(io, snapshot.manifest_list)[0] manifest_entries = demo_manifest_file.fetch_manifest_entry(io) test_schema = Schema( NestedField(1, "VendorID", IntegerType(), False), NestedField(2, "tpep_pickup_datetime", IntegerType(), False) @@ -508,7 +508,7 @@ def test_write_manifest_list( schema_id=3, ) - demo_manifest_list = snapshot.manifests(io) + demo_manifest_list = snapshot.manifests(io, snapshot.manifest_list) with TemporaryDirectory() as tmp_dir: path = tmp_dir + "/manifest-list.avro" output = io.new_output(path) From fc986487fdf86ebc0ce64a3c1a77aa1f21b326f6 Mon Sep 17 00:00:00 2001 From: chinmay-bhat <12948588+chinmay-bhat@users.noreply.github.com> Date: Tue, 4 Jun 2024 12:59:11 +0530 Subject: [PATCH 2/5] update API --- pyiceberg/catalog/__init__.py | 2 +- pyiceberg/cli/output.py | 2 +- pyiceberg/table/snapshots.py | 6 +++++- tests/integration/test_partitioning_key.py | 8 ++------ tests/integration/test_rest_manifest.py | 2 +- tests/utils/test_manifest.py | 8 ++++---- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py index c2aa39a70e..68583bf3c9 100644 --- a/pyiceberg/catalog/__init__.py +++ b/pyiceberg/catalog/__init__.py @@ -835,7 +835,7 @@ def purge_table(self, identifier: Union[str, Identifier]) -> None: manifest_lists_to_delete = set() manifests_to_delete: List[ManifestFile] = [] for snapshot in metadata.snapshots: - manifests_to_delete += snapshot.manifests(io, snapshot.manifest_list) + manifests_to_delete += snapshot.manifests(io) if snapshot.manifest_list is not None: manifest_lists_to_delete.add(snapshot.manifest_list) diff --git a/pyiceberg/cli/output.py b/pyiceberg/cli/output.py index e9ba377453..56b544c99f 100644 --- a/pyiceberg/cli/output.py +++ b/pyiceberg/cli/output.py @@ -144,7 +144,7 @@ def files(self, table: Table, history: bool) -> None: manifest_list_str = f": {snapshot.manifest_list}" if snapshot.manifest_list else "" list_tree = snapshot_tree.add(f"Snapshot {snapshot.snapshot_id}, schema {snapshot.schema_id}{manifest_list_str}") - manifest_list = snapshot.manifests(io, manifest_list_str) + manifest_list = snapshot.manifests(io) for manifest in manifest_list: manifest_tree = list_tree.add(f"Manifest: {manifest.manifest_path}") for manifest_entry in manifest.fetch_manifest_entry(io, discard_deleted=False): diff --git a/pyiceberg/table/snapshots.py b/pyiceberg/table/snapshots.py index 7268e63ac1..aadeea6f06 100644 --- a/pyiceberg/table/snapshots.py +++ b/pyiceberg/table/snapshots.py @@ -252,13 +252,17 @@ def __str__(self) -> str: @staticmethod @lru_cache - def manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]: + def _manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]: """Return the manifests for the given snapshot.""" if manifest_list not in (None, ""): file = io.new_input(manifest_list) return list(read_manifest_list(file)) return [] + def manifests(self, io: FileIO) -> List[ManifestFile]: + """Return the manifests for the given snapshot.""" + return Snapshot._manifests(io, self.manifest_list) + class MetadataLogEntry(IcebergBaseModel): metadata_file: str = Field(alias="metadata-file") diff --git a/tests/integration/test_partitioning_key.py b/tests/integration/test_partitioning_key.py index f45223a224..29f664909c 100644 --- a/tests/integration/test_partitioning_key.py +++ b/tests/integration/test_partitioning_key.py @@ -763,14 +763,10 @@ def test_partition_key( snapshot = iceberg_table.current_snapshot() assert snapshot spark_partition_for_justification = ( - snapshot.manifests(iceberg_table.io, snapshot.manifest_list)[0] - .fetch_manifest_entry(iceberg_table.io)[0] - .data_file.partition + snapshot.manifests(iceberg_table.io)[0].fetch_manifest_entry(iceberg_table.io)[0].data_file.partition ) spark_path_for_justification = ( - snapshot.manifests(iceberg_table.io, snapshot.manifest_list)[0] - .fetch_manifest_entry(iceberg_table.io)[0] - .data_file.file_path + snapshot.manifests(iceberg_table.io)[0].fetch_manifest_entry(iceberg_table.io)[0].data_file.file_path ) assert spark_partition_for_justification == expected_partition_record assert expected_hive_partition_path_slice in spark_path_for_justification diff --git a/tests/integration/test_rest_manifest.py b/tests/integration/test_rest_manifest.py index 8c4b2aaf64..82c41cfd93 100644 --- a/tests/integration/test_rest_manifest.py +++ b/tests/integration/test_rest_manifest.py @@ -75,7 +75,7 @@ def test_write_sample_manifest(table_test_all_types: Table) -> None: if test_snapshot is None: raise ValueError("Table has no current snapshot, check the docker environment") io = table_test_all_types.io - test_manifest_file = test_snapshot.manifests(io, test_snapshot.manifest_list)[0] + test_manifest_file = test_snapshot.manifests(io)[0] test_manifest_entries = test_manifest_file.fetch_manifest_entry(io) entry = test_manifest_entries[0] test_schema = table_test_all_types.schema() diff --git a/tests/utils/test_manifest.py b/tests/utils/test_manifest.py index 859264b0d3..ef33b16b00 100644 --- a/tests/utils/test_manifest.py +++ b/tests/utils/test_manifest.py @@ -217,7 +217,7 @@ def test_read_manifest_v1(generated_manifest_file_file_v1: str) -> None: summary=Summary(Operation.APPEND), schema_id=3, ) - manifest_list = snapshot.manifests(io, snapshot.manifest_list)[0] + manifest_list = snapshot.manifests(io)[0] assert manifest_list.manifest_length == 7989 assert manifest_list.partition_spec_id == 0 @@ -267,7 +267,7 @@ def test_read_manifest_v2(generated_manifest_file_file_v2: str) -> None: summary=Summary(Operation.APPEND), schema_id=3, ) - manifest_list = snapshot.manifests(io, manifest_list=snapshot.manifest_list)[0] + manifest_list = snapshot.manifests(io)[0] assert manifest_list.manifest_length == 7989 assert manifest_list.partition_spec_id == 0 @@ -336,7 +336,7 @@ def test_write_manifest( summary=Summary(Operation.APPEND), schema_id=3, ) - demo_manifest_file = snapshot.manifests(io, snapshot.manifest_list)[0] + demo_manifest_file = snapshot.manifests(io)[0] manifest_entries = demo_manifest_file.fetch_manifest_entry(io) test_schema = Schema( NestedField(1, "VendorID", IntegerType(), False), NestedField(2, "tpep_pickup_datetime", IntegerType(), False) @@ -508,7 +508,7 @@ def test_write_manifest_list( schema_id=3, ) - demo_manifest_list = snapshot.manifests(io, snapshot.manifest_list) + demo_manifest_list = snapshot.manifests(io) with TemporaryDirectory() as tmp_dir: path = tmp_dir + "/manifest-list.avro" output = io.new_output(path) From 00e2f44e231bf6c7daf34465df34fc8aca68d5e1 Mon Sep 17 00:00:00 2001 From: chinmay-bhat <12948588+chinmay-bhat@users.noreply.github.com> Date: Tue, 4 Jun 2024 13:30:59 +0530 Subject: [PATCH 3/5] small fix --- pyiceberg/table/snapshots.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyiceberg/table/snapshots.py b/pyiceberg/table/snapshots.py index aadeea6f06..d0d6f637a0 100644 --- a/pyiceberg/table/snapshots.py +++ b/pyiceberg/table/snapshots.py @@ -252,10 +252,10 @@ def __str__(self) -> str: @staticmethod @lru_cache - def _manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]: + def _manifests(io: FileIO, manifest_list: Optional[str]) -> List[ManifestFile]: """Return the manifests for the given snapshot.""" if manifest_list not in (None, ""): - file = io.new_input(manifest_list) + file = io.new_input(manifest_list) # type: ignore return list(read_manifest_list(file)) return [] From 1df0f0318e0fe99408c2e51c4f2dfc1ac0e915aa Mon Sep 17 00:00:00 2001 From: chinmay-bhat <12948588+chinmay-bhat@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:31:44 +0530 Subject: [PATCH 4/5] move cache to module level --- pyiceberg/table/snapshots.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pyiceberg/table/snapshots.py b/pyiceberg/table/snapshots.py index d0d6f637a0..aa72484dc9 100644 --- a/pyiceberg/table/snapshots.py +++ b/pyiceberg/table/snapshots.py @@ -231,6 +231,15 @@ def __eq__(self, other: Any) -> bool: ) +@lru_cache +def _manifests(io: FileIO, manifest_list: Optional[str]) -> List[ManifestFile]: + """Return the manifests from the manifest list.""" + if manifest_list not in (None, ""): + file = io.new_input(manifest_list) # type: ignore + return list(read_manifest_list(file)) + return [] + + class Snapshot(IcebergBaseModel): snapshot_id: int = Field(alias="snapshot-id") parent_snapshot_id: Optional[int] = Field(alias="parent-snapshot-id", default=None) @@ -250,18 +259,9 @@ def __str__(self) -> str: result_str = f"{operation}id={self.snapshot_id}{parent_id}{schema_id}" return result_str - @staticmethod - @lru_cache - def _manifests(io: FileIO, manifest_list: Optional[str]) -> List[ManifestFile]: - """Return the manifests for the given snapshot.""" - if manifest_list not in (None, ""): - file = io.new_input(manifest_list) # type: ignore - return list(read_manifest_list(file)) - return [] - def manifests(self, io: FileIO) -> List[ManifestFile]: """Return the manifests for the given snapshot.""" - return Snapshot._manifests(io, self.manifest_list) + return _manifests(io, self.manifest_list) class MetadataLogEntry(IcebergBaseModel): From a54c57077185a8345b2c2537aa691e364f79caa9 Mon Sep 17 00:00:00 2001 From: chinmay-bhat <12948588+chinmay-bhat@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:58:02 +0530 Subject: [PATCH 5/5] update signature and check --- pyiceberg/table/snapshots.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pyiceberg/table/snapshots.py b/pyiceberg/table/snapshots.py index aa72484dc9..980399a2ab 100644 --- a/pyiceberg/table/snapshots.py +++ b/pyiceberg/table/snapshots.py @@ -232,12 +232,10 @@ def __eq__(self, other: Any) -> bool: @lru_cache -def _manifests(io: FileIO, manifest_list: Optional[str]) -> List[ManifestFile]: +def _manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]: """Return the manifests from the manifest list.""" - if manifest_list not in (None, ""): - file = io.new_input(manifest_list) # type: ignore - return list(read_manifest_list(file)) - return [] + file = io.new_input(manifest_list) + return list(read_manifest_list(file)) class Snapshot(IcebergBaseModel): @@ -261,7 +259,9 @@ def __str__(self) -> str: def manifests(self, io: FileIO) -> List[ManifestFile]: """Return the manifests for the given snapshot.""" - return _manifests(io, self.manifest_list) + if self.manifest_list: + return _manifests(io, self.manifest_list) + return [] class MetadataLogEntry(IcebergBaseModel):