sungwy commented on code in PR #1112:
URL: https://github.com/apache/iceberg-python/pull/1112#discussion_r1736166915
##########
pyiceberg/catalog/hive.py:
##########
@@ -445,7 +449,9 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
if lock.state == LockState.WAITING:
self._wait_for_lock(database_name, table_name,
lock.lockid, open_client)
else:
- raise CommitFailedException(f"Failed to acquire lock
for {table_request.identifier}, state: {lock.state}")
+ raise CommitFailedException(
+ f"Failed to acquire lock for
{table_identifier.identifier}, state: {lock.state}"
Review Comment:
```suggestion
f"Failed to acquire lock for {table_identifier},
state: {lock.state}"
```
##########
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)
Review Comment:
How do you feel about using the private equivalent method
`self._identifier_to_tuple_without_catalog` instead?
The public method will result in commit_table always throwing a deprecation
warning, whereas the private method will throw a warning only if the catalog
name is included in table.identifier.
https://github.com/apache/iceberg-python/blob/ba8e9a368a8c1a2798b14c3ebdca653a1c2d52b5/pyiceberg/catalog/__init__.py#L635-L652
My apologies in advance that having two similarly named methods is
confusing... But given that `identifier_to_tuple_without_catalog` was already a
public method, we didn't have much choice but to have these two versions of the
method until 0.9.0 so that each `_commit_table` didn't raise a deprecation
warning :)
##########
pyiceberg/catalog/hive.py:
##########
@@ -486,7 +492,7 @@ def _commit_table(self, table_request: CommitTableRequest)
-> CommitTableRespons
)
self._create_hive_table(open_client, hive_table)
except WaitingForLockException as e:
- raise CommitFailedException(f"Failed to acquire lock for
{table_request.identifier}, state: {lock.state}") from e
+ raise CommitFailedException(f"Failed to acquire lock for
{table.identifier}, state: {lock.state}") from e
Review Comment:
```suggestion
raise CommitFailedException(f"Failed to acquire lock for
{table_identifier}, state: {lock.state}") from e
```
##########
pyiceberg/catalog/rest.py:
##########
@@ -734,9 +739,11 @@ 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 = TableIdentifier(namespace=table.identifier[1:-1],
name=table.identifier[-1])
Review Comment:
Would using `self._identifier_to_tuple_without_catalog` until the use of
catalog name is deprecated in 0.9.0 be safer than removing the first identifier
in all cases?
##########
pyiceberg/catalog/rest.py:
##########
@@ -719,12 +721,15 @@ def
_remove_catalog_name_from_table_request_identifier(self, table_request: Comm
)
return table_request
- @retry(**_RETRY_ARGS)
Review Comment:
Did we remove the retry decorator intentionally?
--
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
