sungwy commented on code in PR #820:
URL: https://github.com/apache/iceberg-python/pull/820#discussion_r1736248307
##########
pyiceberg/catalog/rest.py:
##########
@@ -379,17 +382,18 @@ def _fetch_config(self) -> None:
def _identifier_to_validated_tuple(self, identifier: Union[str,
Identifier]) -> Identifier:
identifier_tuple = self.identifier_to_tuple(identifier)
if len(identifier_tuple) <= 1:
- raise NoSuchTableError(f"Missing namespace or invalid identifier:
{'.'.join(identifier_tuple)}")
+ raise NoSuchIdentifierError(f"Missing namespace or invalid
identifier: {'.'.join(identifier_tuple)}")
return identifier_tuple
- def _split_identifier_for_path(self, identifier: Union[str, Identifier,
TableIdentifier]) -> Properties:
+ def _split_identifier_for_path(self, identifier: Union[str, Identifier,
TableIdentifier], kind: str = "table") -> Properties:
Review Comment:
nit: Should we introduce an enum instead of free form strings for the table
kind? (table, view, there may be more kinds in the future like materialized
views)
##########
pyiceberg/catalog/rest.py:
##########
@@ -379,17 +382,18 @@ def _fetch_config(self) -> None:
def _identifier_to_validated_tuple(self, identifier: Union[str,
Identifier]) -> Identifier:
identifier_tuple = self.identifier_to_tuple(identifier)
if len(identifier_tuple) <= 1:
- raise NoSuchTableError(f"Missing namespace or invalid identifier:
{'.'.join(identifier_tuple)}")
+ raise NoSuchIdentifierError(f"Missing namespace or invalid
identifier: {'.'.join(identifier_tuple)}")
return identifier_tuple
- def _split_identifier_for_path(self, identifier: Union[str, Identifier,
TableIdentifier]) -> Properties:
+ def _split_identifier_for_path(self, identifier: Union[str, Identifier,
TableIdentifier], kind: str = "table") -> Properties:
if isinstance(identifier, TableIdentifier):
if identifier.namespace.root[0] == self.name:
return {"namespace":
NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), "table":
identifier.name}
else:
return {"namespace":
NAMESPACE_SEPARATOR.join(identifier.namespace.root), "table": identifier.name}
Review Comment:
The [view identifier is also of
TableIdentifier](https://github.com/apache/iceberg/blob/6c7964002b097b933053d8f6db63c0851a55a39a/open-api/rest-catalog-open-api.yaml#L2886C39-L2886C54)
type, so I think this would be safer, and future proof (for when we support
loading views):
```suggestion
return {"namespace":
NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), kind: identifier.name}
else:
return {"namespace":
NAMESPACE_SEPARATOR.join(identifier.namespace.root), kind: identifier.name}
```
--
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
