kevinjqliu commented on code in PR #1112:
URL: https://github.com/apache/iceberg-python/pull/1112#discussion_r1739511962
##########
pyiceberg/table/__init__.py:
##########
@@ -1673,12 +1673,8 @@ def refs(self) -> Dict[str, SnapshotRef]:
return self.metadata.refs
def _do_commit(self, updates: Tuple[TableUpdate, ...], requirements:
Tuple[TableRequirement, ...]) -> None:
- response = self.catalog._commit_table( # pylint: disable=W0212
- CommitTableRequest(
- identifier=TableIdentifier(namespace=self._identifier[:-1],
name=self._identifier[-1]),
- updates=updates,
- requirements=requirements,
- )
+ response = self.catalog.commit_table( # pylint: disable=W0212
+ self, requirements, updates
) # pylint: disable=W0212
Review Comment:
```suggestion
)
```
here as well
##########
pyiceberg/table/__init__.py:
##########
@@ -1673,12 +1673,8 @@ def refs(self) -> Dict[str, SnapshotRef]:
return self.metadata.refs
def _do_commit(self, updates: Tuple[TableUpdate, ...], requirements:
Tuple[TableRequirement, ...]) -> None:
- response = self.catalog._commit_table( # pylint: disable=W0212
- CommitTableRequest(
- identifier=TableIdentifier(namespace=self._identifier[:-1],
name=self._identifier[-1]),
- updates=updates,
- requirements=requirements,
- )
+ response = self.catalog.commit_table( # pylint: disable=W0212
Review Comment:
```suggestion
response = self.catalog.commit_table(
```
i think we can get rid of disable pylint here since its no longer a private
method
https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/protected-access.html
##########
pyiceberg/catalog/hive.py:
##########
@@ -434,10 +440,8 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
NoSuchTableError: If a table with the given identifier does not
exist.
CommitFailedException: Requirement not met, or a conflict with a
concurrent commit.
"""
- identifier_tuple = self._identifier_to_tuple_without_catalog(
- tuple(table_request.identifier.namespace.root +
[table_request.identifier.name])
- )
- database_name, table_name =
self.identifier_to_database_and_table(identifier_tuple, NoSuchTableError)
+ table_identifier =
self._identifier_to_tuple_without_catalog(table.identifier)
+ database_name, table_name = table_identifier
Review Comment:
also maybe use `identifier_to_database_and_table`
##########
pyiceberg/catalog/glue.py:
##########
@@ -462,10 +468,8 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
NoSuchTableError: If a table with the given identifier does not
exist.
CommitFailedException: Requirement not met, or a conflict with a
concurrent commit.
"""
- identifier_tuple = self._identifier_to_tuple_without_catalog(
- tuple(table_request.identifier.namespace.root +
[table_request.identifier.name])
- )
- database_name, table_name =
self.identifier_to_database_and_table(identifier_tuple)
+ table_identifier =
self._identifier_to_tuple_without_catalog(table.identifier)
+ database_name, table_name = table_identifier
Review Comment:
maybe use `identifier_to_database_and_table` here?
https://github.com/apache/iceberg-python/blob/c647256b1d9a278a88f5e757bde4db76017d47fd/pyiceberg/catalog/glue.py#L549
##########
pyiceberg/catalog/rest.py:
##########
@@ -755,9 +750,12 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
CommitFailedException: Requirement not met, or a conflict with a
concurrent commit.
CommitStateUnknownException: Failed due to an internal exception
on the side of the catalog.
"""
+ identifier =
self._identifier_to_tuple_without_catalog(table.identifier)
+ table_identifier = TableIdentifier(namespace=identifier[0:-1],
name=identifier[-1])
Review Comment:
```suggestion
table_identifier = TableIdentifier(namespace=identifier[:-1],
name=identifier[-1])
```
nit: more pythonic and also mirrors
https://github.com/apache/iceberg-python/blob/e4c1748fee220076f04e35ab2f182dd51ca20705/pyiceberg/table/__init__.py#L1678
##########
pyiceberg/catalog/rest.py:
##########
@@ -773,6 +771,17 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
)
return CommitTableResponse(**response.json())
+ @retry(**_RETRY_ARGS)
+ def list_views(self, namespace: Union[str, Identifier]) ->
List[Identifier]:
Review Comment:
nit: move `list_views` above `commit_table` for better github diff
##########
tests/catalog/test_base.py:
##########
@@ -670,14 +670,13 @@ def test_commit_table(catalog: InMemoryCatalog) -> None:
)
# When
- response = given_table.catalog._commit_table( # pylint: disable=W0212
- CommitTableRequest(
-
identifier=TableIdentifier(namespace=Namespace(given_table._identifier[:-1]),
name=given_table._identifier[-1]),
- updates=[
- AddSchemaUpdate(schema=new_schema,
last_column_id=new_schema.highest_field_id),
- SetCurrentSchemaUpdate(schema_id=-1),
- ],
- )
+ response = given_table.catalog.commit_table( # pylint: disable=W0212
Review Comment:
```suggestion
response = given_table.catalog.commit_table(
```
can remove disable pylint
##########
pyiceberg/catalog/sql.py:
##########
@@ -407,20 +412,18 @@ def _commit_table(self, table_request:
CommitTableRequest) -> CommitTableRespons
NoSuchTableError: If a table with the given identifier does not
exist.
CommitFailedException: Requirement not met, or a conflict with a
concurrent commit.
"""
- identifier_tuple = self._identifier_to_tuple_without_catalog(
- tuple(table_request.identifier.namespace.root +
[table_request.identifier.name])
- )
- namespace_tuple = Catalog.namespace_from(identifier_tuple)
+ table_identifier =
self._identifier_to_tuple_without_catalog(table.identifier)
+ namespace_tuple = Catalog.namespace_from(table_identifier)
namespace = Catalog.namespace_to_string(namespace_tuple)
- table_name = Catalog.table_name_from(identifier_tuple)
+ table_name = Catalog.table_name_from(table_identifier)
current_table: Optional[Table]
try:
- current_table = self.load_table(identifier_tuple)
+ current_table = self.load_table(table_identifier)
except NoSuchTableError:
current_table = None
- updated_staged_table = self._update_and_stage_table(current_table,
table_request)
+ updated_staged_table = self._update_and_stage_table(current_table,
table.identifier[1:], requirements, updates)
Review Comment:
```suggestion
updated_staged_table = self._update_and_stage_table(current_table,
table.identifier, requirements, updates)
```
can just use `table_identifier` here
--
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
