Fokko commented on code in PR #787:
URL: https://github.com/apache/iceberg-python/pull/787#discussion_r1632371435
##########
pyiceberg/table/snapshots.py:
##########
@@ -247,12 +248,19 @@ 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: Optional[str]) ->
List[ManifestFile]:
Review Comment:
I think it should not be part of the `Snapshot` class. This class gets
recreated, but then it will also invalidate the cache. The idea behind the LRU
cache is that you keep it around until it grows to a certain size, and then you
start dropping the last used entries.
If we move it to the top of the file, the cache continues to work across
snapshots:
```suggestion
def _manifests(io: FileIO, manifest_list: Optional[str]) ->
List[ManifestFile]:
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
