Re: [PR] Add drop_view to the rest catalog [iceberg-python]

["sungwy (via GitHub)" <gi...@apache.org>]

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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org