kevinjqliu commented on code in PR #1642:
URL: https://github.com/apache/iceberg-python/pull/1642#discussion_r1951696715
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -84,14 +84,14 @@
from pyiceberg.table import Transaction
-def _new_manifest_path(location: str, num: int, commit_uuid: uuid.UUID) -> str:
- return f"{location}/metadata/{commit_uuid}-m{num}.avro"
+def _new_manifest_file_name(num: int, commit_uuid: uuid.UUID) -> str:
+ return f"{commit_uuid}-m{num}.avro"
-def _generate_manifest_list_path(location: str, snapshot_id: int, attempt:
int, commit_uuid: uuid.UUID) -> str:
+def _generate_manifest_list_file_name(snapshot_id: int, attempt: int,
commit_uuid: uuid.UUID) -> str:
Review Comment:
nit:
```suggestion
def _new_manifest_list_file_name(snapshot_id: int, attempt: int,
commit_uuid: uuid.UUID) -> str:
```
##########
pyiceberg/table/__init__.py:
##########
@@ -1212,6 +1213,23 @@ def to_daft(self) -> daft.DataFrame:
return daft.read_iceberg(self)
+ @staticmethod
+ def metadata_file_location(table_location: str, file_name: str,
properties: Properties = EMPTY_DICT) -> str:
+ """Get the full path for a metadata file.
+
+ Args:
+ table_location (str): The base table location
+ file_name (str): Name of the metadata file
+ properties (Properties): Table properties that may contain custom
metadata path
+
+ Returns:
+ str: Full path where the metadata file should be stored
+ """
Review Comment:
nit: what if we model it like `new_data_location`
https://github.com/apache/iceberg-python/blob/509713bbc23468008eb9e5ac659db9dbb6828c6d/pyiceberg/table/locations.py#L55-L65
```suggestion
@staticmethod
def new_metadata_location(table_location: str, file_name: str,
properties: Properties = EMPTY_DICT) -> str:
"""Return a fully-qualified metadata file location for the given
filename.
Args:
table_location (str): The base table location
file_name (str): Name of the metadata file
properties (Properties): Table properties that may contain
custom metadata path
Returns:
str: A fully-qualified location URI for the metadata file.
"""
```
##########
pyiceberg/catalog/__init__.py:
##########
@@ -929,11 +931,12 @@ def _write_metadata(metadata: TableMetadata, io: FileIO,
metadata_path: str) ->
ToOutputFile.table_metadata(metadata, io.new_output(metadata_path))
@staticmethod
- def _get_metadata_location(location: str, new_version: int = 0) -> str:
+ def _get_metadata_location(table_location: str, new_version: int = 0,
properties: Properties = EMPTY_DICT) -> str:
Review Comment:
do you think we can just get rid of this function entirely and replace with
the underlying table function? (`Table.metadata_file_location`)
--
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]
