From 9881f11dec9e079b273702ef55f73670816489e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=20Anast=C3=A1cio?= Date: Fri, 14 Jun 2024 13:08:01 -0300 Subject: [PATCH 1/3] Use NoSuchIdentifier instead of NoTableIdentifier This change allows for the validation of both Tables and Views. --- pyiceberg/catalog/rest.py | 3 ++- pyiceberg/exceptions.py | 6 ++++++ tests/catalog/test_rest.py | 9 +++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/pyiceberg/catalog/rest.py b/pyiceberg/catalog/rest.py index a7a19d1014..0bc500f2c3 100644 --- a/pyiceberg/catalog/rest.py +++ b/pyiceberg/catalog/rest.py @@ -56,6 +56,7 @@ ServiceUnavailableError, TableAlreadyExistsError, UnauthorizedError, + NoSuchIdentifierError, ) from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC, PartitionSpec, assign_fresh_partition_spec_ids from pyiceberg.schema import Schema, assign_fresh_schema_ids @@ -389,7 +390,7 @@ 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: diff --git a/pyiceberg/exceptions.py b/pyiceberg/exceptions.py index c7e37ba7ca..981a459d63 100644 --- a/pyiceberg/exceptions.py +++ b/pyiceberg/exceptions.py @@ -40,6 +40,12 @@ class NoSuchIcebergTableError(NoSuchTableError): """Raises when the table found in the REST catalog is not an iceberg table.""" + + +class NoSuchIdentifierError(Exception): + """Raises when the identifier can't be found in the REST catalog.""" + + class NoSuchNamespaceError(Exception): """Raised when a referenced name-space is not found.""" diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 470f60c277..7c884221f1 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -34,6 +34,7 @@ OAuthError, ServerError, TableAlreadyExistsError, + NoSuchIdentifierError, ) from pyiceberg.io import load_file_io from pyiceberg.partitioning import PartitionField, PartitionSpec @@ -1158,7 +1159,7 @@ def test_delete_table_404(rest_mock: Mocker) -> None: def test_create_table_missing_namespace(rest_mock: Mocker, table_schema_simple: Schema) -> None: table = "table" - with pytest.raises(NoSuchTableError) as e: + with pytest.raises(NoSuchIdentifierError) as e: # Missing namespace RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).create_table(table, table_schema_simple) assert f"Missing namespace or invalid identifier: {table}" in str(e.value) @@ -1166,7 +1167,7 @@ def test_create_table_missing_namespace(rest_mock: Mocker, table_schema_simple: def test_load_table_invalid_namespace(rest_mock: Mocker) -> None: table = "table" - with pytest.raises(NoSuchTableError) as e: + with pytest.raises(NoSuchIdentifierError) as e: # Missing namespace RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).load_table(table) assert f"Missing namespace or invalid identifier: {table}" in str(e.value) @@ -1174,7 +1175,7 @@ def test_load_table_invalid_namespace(rest_mock: Mocker) -> None: def test_drop_table_invalid_namespace(rest_mock: Mocker) -> None: table = "table" - with pytest.raises(NoSuchTableError) as e: + with pytest.raises(NoSuchIdentifierError) as e: # Missing namespace RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).drop_table(table) assert f"Missing namespace or invalid identifier: {table}" in str(e.value) @@ -1182,7 +1183,7 @@ def test_drop_table_invalid_namespace(rest_mock: Mocker) -> None: def test_purge_table_invalid_namespace(rest_mock: Mocker) -> None: table = "table" - with pytest.raises(NoSuchTableError) as e: + with pytest.raises(NoSuchIdentifierError) as e: # Missing namespace RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).purge_table(table) assert f"Missing namespace or invalid identifier: {table}" in str(e.value) From 275662546a23539cd21e5679d2d00ca826178d5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=20Anast=C3=A1cio?= Date: Fri, 16 Aug 2024 10:19:14 -0300 Subject: [PATCH 2/3] Add drop_view to the rest catalog --- pyiceberg/catalog/__init__.py | 11 ++++++++++ pyiceberg/catalog/dynamodb.py | 3 +++ pyiceberg/catalog/glue.py | 3 +++ pyiceberg/catalog/hive.py | 3 +++ pyiceberg/catalog/noop.py | 3 +++ pyiceberg/catalog/rest.py | 20 ++++++++++++++--- pyiceberg/catalog/sql.py | 3 +++ pyiceberg/exceptions.py | 2 ++ tests/catalog/test_base.py | 3 +++ tests/catalog/test_rest.py | 41 ++++++++++++++++++++++++++++++++++- 10 files changed, 88 insertions(+), 4 deletions(-) diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py index 71ed911466..690e9c4a40 100644 --- a/pyiceberg/catalog/__init__.py +++ b/pyiceberg/catalog/__init__.py @@ -629,6 +629,17 @@ def update_namespace_properties( ValueError: If removals and updates have overlapping keys. """ + @abstractmethod + def drop_view(self, identifier: Union[str, Identifier]) -> None: + """Drop a view. + + Args: + identifier (str | Identifier): View identifier. + + Raises: + NoSuchViewError: If a view with the given name does not exist. + """ + @deprecated( deprecated_in="0.8.0", removed_in="0.9.0", diff --git a/pyiceberg/catalog/dynamodb.py b/pyiceberg/catalog/dynamodb.py index 07d9d6938c..b308678826 100644 --- a/pyiceberg/catalog/dynamodb.py +++ b/pyiceberg/catalog/dynamodb.py @@ -531,6 +531,9 @@ def update_namespace_properties( def list_views(self, namespace: Union[str, Identifier]) -> List[Identifier]: raise NotImplementedError + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError + def _get_iceberg_table_item(self, database_name: str, table_name: str) -> Dict[str, Any]: try: return self._get_dynamo_item(identifier=f"{database_name}.{table_name}", namespace=database_name) diff --git a/pyiceberg/catalog/glue.py b/pyiceberg/catalog/glue.py index 2c3082b7b4..05990325d2 100644 --- a/pyiceberg/catalog/glue.py +++ b/pyiceberg/catalog/glue.py @@ -772,3 +772,6 @@ def update_namespace_properties( def list_views(self, namespace: Union[str, Identifier]) -> List[Identifier]: raise NotImplementedError + + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError diff --git a/pyiceberg/catalog/hive.py b/pyiceberg/catalog/hive.py index 096cfc1478..ce725ccd23 100644 --- a/pyiceberg/catalog/hive.py +++ b/pyiceberg/catalog/hive.py @@ -707,3 +707,6 @@ def update_namespace_properties( expected_to_change = (removals or set()).difference(removed) return PropertiesUpdateSummary(removed=list(removed or []), updated=list(updated or []), missing=list(expected_to_change)) + + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError diff --git a/pyiceberg/catalog/noop.py b/pyiceberg/catalog/noop.py index eaa5e289a1..0f16b6909f 100644 --- a/pyiceberg/catalog/noop.py +++ b/pyiceberg/catalog/noop.py @@ -116,3 +116,6 @@ def update_namespace_properties( def list_views(self, namespace: Union[str, Identifier]) -> List[Identifier]: raise NotImplementedError + + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError diff --git a/pyiceberg/catalog/rest.py b/pyiceberg/catalog/rest.py index 0bc500f2c3..28d1a85da5 100644 --- a/pyiceberg/catalog/rest.py +++ b/pyiceberg/catalog/rest.py @@ -48,15 +48,16 @@ ForbiddenError, NamespaceAlreadyExistsError, NamespaceNotEmptyError, + NoSuchIdentifierError, NoSuchNamespaceError, NoSuchTableError, + NoSuchViewError, OAuthError, RESTError, ServerError, ServiceUnavailableError, TableAlreadyExistsError, UnauthorizedError, - NoSuchIdentifierError, ) from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC, PartitionSpec, assign_fresh_partition_spec_ids from pyiceberg.schema import Schema, assign_fresh_schema_ids @@ -98,6 +99,7 @@ class Endpoints: get_token: str = "oauth/tokens" rename_table: str = "tables/rename" list_views: str = "namespaces/{namespace}/views" + drop_view: str = "namespaces/{namespace}/views/{view}" AUTHORIZATION_HEADER = "Authorization" @@ -393,14 +395,15 @@ def _identifier_to_validated_tuple(self, identifier: Union[str, Identifier]) -> 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} identifier_tuple = self._identifier_to_validated_tuple(identifier) - return {"namespace": NAMESPACE_SEPARATOR.join(identifier_tuple[:-1]), "table": identifier_tuple[-1]} + + return {"namespace": NAMESPACE_SEPARATOR.join(identifier_tuple[:-1]), kind: identifier_tuple[-1]} def _split_identifier_for_json(self, identifier: Union[str, Identifier]) -> Dict[str, Union[Identifier, str]]: identifier_tuple = self._identifier_to_validated_tuple(identifier) @@ -868,3 +871,14 @@ def table_exists(self, identifier: Union[str, Identifier]) -> bool: self._handle_non_200_response(exc, {}) return False + + @retry(**_RETRY_ARGS) + def drop_view(self, identifier: Union[str]) -> None: + identifier_tuple = self.identifier_to_tuple_without_catalog(identifier) + response = self._session.delete( + self.url(Endpoints.drop_view, prefixed=True, **self._split_identifier_for_path(identifier_tuple, kind="view")), + ) + try: + response.raise_for_status() + except HTTPError as exc: + self._handle_non_200_response(exc, {404: NoSuchViewError}) diff --git a/pyiceberg/catalog/sql.py b/pyiceberg/catalog/sql.py index 5b9e697ec3..c7e546ba2b 100644 --- a/pyiceberg/catalog/sql.py +++ b/pyiceberg/catalog/sql.py @@ -699,3 +699,6 @@ def update_namespace_properties( def list_views(self, namespace: Union[str, Identifier]) -> List[Identifier]: raise NotImplementedError + + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError diff --git a/pyiceberg/exceptions.py b/pyiceberg/exceptions.py index 981a459d63..56574ff471 100644 --- a/pyiceberg/exceptions.py +++ b/pyiceberg/exceptions.py @@ -40,6 +40,8 @@ class NoSuchIcebergTableError(NoSuchTableError): """Raises when the table found in the REST catalog is not an iceberg table.""" +class NoSuchViewError(Exception): + """Raises when the view can't be found in the REST catalog.""" class NoSuchIdentifierError(Exception): diff --git a/tests/catalog/test_base.py b/tests/catalog/test_base.py index 095d93464f..e87de9b1ab 100644 --- a/tests/catalog/test_base.py +++ b/tests/catalog/test_base.py @@ -259,6 +259,9 @@ def update_namespace_properties( def list_views(self, namespace: Optional[Union[str, Identifier]] = None) -> List[Identifier]: raise NotImplementedError + def drop_view(self, identifier: Union[str, Identifier]) -> None: + raise NotImplementedError + @pytest.fixture def catalog(tmp_path: PosixPath) -> InMemoryCatalog: diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index 7c884221f1..d7b5b673b9 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -29,12 +29,13 @@ AuthorizationExpiredError, NamespaceAlreadyExistsError, NamespaceNotEmptyError, + NoSuchIdentifierError, NoSuchNamespaceError, NoSuchTableError, + NoSuchViewError, OAuthError, ServerError, TableAlreadyExistsError, - NoSuchIdentifierError, ) from pyiceberg.io import load_file_io from pyiceberg.partitioning import PartitionField, PartitionSpec @@ -1308,3 +1309,41 @@ def test_table_identifier_in_commit_table_request(rest_mock: Mocker, example_tab rest_mock.last_request.text == """{"identifier":{"namespace":["namespace"],"name":"table_name"},"requirements":[],"updates":[]}""" ) + + +def test_drop_view_invalid_namespace(rest_mock: Mocker) -> None: + view = "view" + with pytest.raises(NoSuchIdentifierError) as e: + # Missing namespace + RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).drop_view(view) + + assert f"Missing namespace or invalid identifier: {view}" in str(e.value) + + +def test_drop_view_404(rest_mock: Mocker) -> None: + rest_mock.delete( + f"{TEST_URI}v1/namespaces/some_namespace/views/does_not_exists", + json={ + "error": { + "message": "The given view does not exist", + "type": "NoSuchViewException", + "code": 404, + } + }, + status_code=404, + request_headers=TEST_HEADERS, + ) + + with pytest.raises(NoSuchViewError) as e: + RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).drop_view(("some_namespace", "does_not_exists")) + assert "The given view does not exist" in str(e.value) + + +def test_drop_view_204(rest_mock: Mocker) -> None: + rest_mock.delete( + f"{TEST_URI}v1/namespaces/some_namespace/views/some_view", + json={}, + status_code=204, + request_headers=TEST_HEADERS, + ) + RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).drop_view(("some_namespace", "some_view")) From 6a074785202b5861c25f463e83d189b42491c24d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=20Anast=C3=A1cio?= Date: Fri, 30 Aug 2024 14:54:21 -0300 Subject: [PATCH 3/3] fixup! Add drop_view to the rest catalog --- pyiceberg/catalog/rest.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/pyiceberg/catalog/rest.py b/pyiceberg/catalog/rest.py index 28d1a85da5..cc6d891e63 100644 --- a/pyiceberg/catalog/rest.py +++ b/pyiceberg/catalog/rest.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from enum import Enum from json import JSONDecodeError from typing import ( TYPE_CHECKING, @@ -102,6 +103,11 @@ class Endpoints: drop_view: str = "namespaces/{namespace}/views/{view}" +class IdentifierKind(Enum): + TABLE = "table" + VIEW = "view" + + AUTHORIZATION_HEADER = "Authorization" BEARER_PREFIX = "Bearer" CATALOG_SCOPE = "catalog" @@ -395,15 +401,17 @@ def _identifier_to_validated_tuple(self, identifier: Union[str, Identifier]) -> 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], kind: str = "table") -> Properties: + def _split_identifier_for_path( + self, identifier: Union[str, Identifier, TableIdentifier], kind: IdentifierKind = IdentifierKind.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} + return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root[1:]), kind.value: identifier.name} else: - return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), "table": identifier.name} + return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), kind.value: identifier.name} identifier_tuple = self._identifier_to_validated_tuple(identifier) - return {"namespace": NAMESPACE_SEPARATOR.join(identifier_tuple[:-1]), kind: identifier_tuple[-1]} + return {"namespace": NAMESPACE_SEPARATOR.join(identifier_tuple[:-1]), kind.value: identifier_tuple[-1]} def _split_identifier_for_json(self, identifier: Union[str, Identifier]) -> Dict[str, Union[Identifier, str]]: identifier_tuple = self._identifier_to_validated_tuple(identifier) @@ -876,7 +884,9 @@ def table_exists(self, identifier: Union[str, Identifier]) -> bool: def drop_view(self, identifier: Union[str]) -> None: identifier_tuple = self.identifier_to_tuple_without_catalog(identifier) response = self._session.delete( - self.url(Endpoints.drop_view, prefixed=True, **self._split_identifier_for_path(identifier_tuple, kind="view")), + self.url( + Endpoints.drop_view, prefixed=True, **self._split_identifier_for_path(identifier_tuple, IdentifierKind.VIEW) + ), ) try: response.raise_for_status()