kevinjqliu commented on code in PR #1626:
URL: https://github.com/apache/iceberg-python/pull/1626#discussion_r1947923174
##########
pyiceberg/table/inspect.py:
##########
@@ -523,7 +523,62 @@ def history(self) -> "pa.Table":
return pa.Table.from_pylist(history, schema=history_schema)
- def _files(self, snapshot_id: Optional[int] = None, data_file_filter:
Optional[Set[DataFileContent]] = None) -> "pa.Table":
+ def _files_by_manifest(
+ self, manifest_list: ManifestFile, data_file_filter:
Optional[Set[DataFileContent]] = None
+ ) -> List[Dict[str, Any]]:
+ files: list[dict[str, Any]] = []
+ schema = self.tbl.metadata.schema()
Review Comment:
when time traveling with different snapshots, we shouldnt just use the
current table schema
for context
https://github.com/apache/iceberg-python/issues/1053#issuecomment-2645885731
##########
pyiceberg/table/inspect.py:
##########
@@ -657,3 +669,35 @@ def all_manifests(self) -> "pa.Table":
lambda args: self._generate_manifests_table(*args), [(snapshot,
True) for snapshot in snapshots]
)
return pa.concat_tables(manifests_by_snapshots)
+
+ def _all_files(self, data_file_filter: Optional[Set[DataFileContent]] =
None) -> "pa.Table":
+ import pyarrow as pa
+
+ snapshots = self.tbl.snapshots()
+ if not snapshots:
+ return pa.Table.from_pylist([], schema=self._get_files_schema())
+
+ executor = ExecutorFactory.get_or_create()
+ all_manifest_files_by_snapshot: Iterator[List[ManifestFile]] =
executor.map(
+ lambda args: args[0].manifests(self.tbl.io), [(snapshot,) for
snapshot in snapshots]
+ )
+ all_manifest_files = list(
+ {(manifest.manifest_path, manifest) for manifest_list in
all_manifest_files_by_snapshot for manifest in manifest_list}
+ )
+ all_files_by_manifest: Iterator[List[Dict[str, Any]]] = executor.map(
+ lambda args: self._files_by_manifest(*args), [(manifest,
data_file_filter) for _, manifest in all_manifest_files]
+ )
+ all_files_list = [file for files in all_files_by_manifest for file in
files]
+ return pa.Table.from_pylist(
+ all_files_list,
+ schema=self._get_files_schema(),
+ )
Review Comment:
WDYT about something like this?
Also i would rename `_files_by_manifest` and have it return pa.Table, so we
can skip the flatten and just concat the tables.
```suggestion
manifest_lists = executor.map(
lambda snapshot: snapshot.manifests(self.tbl.io),
snapshots
)
unique_manifests = {
(manifest.manifest_path, manifest)
for manifest_list in manifest_lists
for manifest in manifest_list
}
file_lists = executor.map(
self._files_by_manifest,
[(manifest, data_file_filter) for _, manifest in
unique_manifests]
)
all_files = [
file
for file_list in file_lists
for file in file_list
]
return pa.Table.from_pylist(
all_files,
schema=self._get_files_schema()
)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
