ebyhr commented on code in PR #2149:
URL: https://github.com/apache/iceberg-python/pull/2149#discussion_r3308804750
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1471,6 +1474,28 @@ def drop_view(self, identifier: str | Identifier) ->
None:
except HTTPError as exc:
_handle_non_200_response(exc, {404: NoSuchViewError})
+ @retry(**_RETRY_ARGS)
+ def rename_view(self, from_identifier: str | Identifier, to_identifier:
str | Identifier) -> None:
Review Comment:
Is `self._check_endpoint(Capability.V1_RENAME_VIEW)` missing intentionally?
##########
pyiceberg/catalog/__init__.py:
##########
@@ -744,6 +744,18 @@ def create_view(
ViewAlreadyExistsError: If a view with the name already exists.
"""
+ @abstractmethod
+ def rename_view(self, from_identifier: str | Identifier, to_identifier:
str | Identifier) -> None:
+ """Rename a fully classified view name.
+
+ Args:
+ from_identifier (str | Identifier): Existing view identifier.
+ to_identifier (str | Identifier): New view identifier.
+
+ Raises:
+ NoSuchViewError: If a view with the name does not exist.
Review Comment:
Can we add `ViewAlreadyExistsError`?
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1471,6 +1474,28 @@ def drop_view(self, identifier: str | Identifier) ->
None:
except HTTPError as exc:
_handle_non_200_response(exc, {404: NoSuchViewError})
+ @retry(**_RETRY_ARGS)
+ def rename_view(self, from_identifier: str | Identifier, to_identifier:
str | Identifier) -> None:
+ payload = {
+ "source": self._split_identifier_for_json(from_identifier),
+ "destination": self._split_identifier_for_json(to_identifier),
+ }
+
+ # Ensure source and destination namespaces exist before rename.
+ source_namespace =
self._split_identifier_for_json(from_identifier)["namespace"]
+ dest_namespace =
self._split_identifier_for_path(to_identifier)["namespace"]
Review Comment:
Why do we use `_split_identifier_for_path` (not
`_split_identifier_for_json`)?
##########
tests/catalog/test_rest.py:
##########
@@ -3167,3 +3167,122 @@ def test_load_table_without_storage_credentials(
)
assert actual.metadata.model_dump() == expected.metadata.model_dump()
assert actual == expected
+
+
+def test_rename_view_204(rest_mock: Mocker) -> None:
+ from_identifier = ("some_namespace", "old_view")
+ to_identifier = ("some_namespace", "new_view")
+ rest_mock.head(
+ f"{TEST_URI}v1/namespaces/some_namespace",
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ rest_mock.post(
+ f"{TEST_URI}v1/views/rename",
+ json={
+ "source": {"namespace": ["some_namespace"], "name": "old_view"},
+ "destination": {"namespace": ["some_namespace"], "name":
"new_view"},
+ },
+ status_code=204,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ catalog.rename_view(from_identifier, to_identifier)
+ assert (
+ rest_mock.last_request.text == """{"source": {"namespace":
["some_namespace"], "name": "old_view"}, """
+ """"destination": {"namespace": ["some_namespace"], "name":
"new_view"}}"""
+ )
+
+
+def test_rename_view_404(rest_mock: Mocker) -> None:
+ from_identifier = ("some_namespace", "non_existent_view")
+ to_identifier = ("some_namespace", "new_view")
+ rest_mock.head(
+ f"{TEST_URI}v1/namespaces/some_namespace",
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ rest_mock.post(
+ f"{TEST_URI}v1/views/rename",
+ json={
+ "error": {
+ "message": "View does not exist:
some_namespace.non_existent_view",
+ "type": "NoSuchViewException",
+ "code": 404,
+ }
+ },
+ status_code=404,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ with pytest.raises(NoSuchViewError) as exc_info:
+ catalog.rename_view(from_identifier, to_identifier)
+ assert "View does not exist: some_namespace.non_existent_view" in
str(exc_info.value)
+
+
+def test_rename_view_409(rest_mock: Mocker) -> None:
+ from_identifier = ("some_namespace", "old_view")
+ to_identifier = ("some_namespace", "existing_view")
+ rest_mock.head(
+ f"{TEST_URI}v1/namespaces/some_namespace",
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ rest_mock.post(
+ f"{TEST_URI}v1/views/rename",
+ json={
+ "error": {
+ "message": "View already exists: some_namespace.existing_view",
+ "type": "ViewAlreadyExistsException",
+ "code": 409,
+ }
+ },
+ status_code=409,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ with pytest.raises(ViewAlreadyExistsError) as exc_info:
+ catalog.rename_view(from_identifier, to_identifier)
+ assert "View already exists: some_namespace.existing_view" in
str(exc_info.value)
+
+
+def test_rename_view_source_namespace_does_not_exist(rest_mock: Mocker) ->
None:
+ from_identifier = ("non_existent_namespace", "old_view")
+ to_identifier = ("some_namespace", "new_view")
+
+ rest_mock.head(
+ f"{TEST_URI}v1/namespaces/non_existent_namespace",
+ status_code=404,
+ request_headers=TEST_HEADERS,
+ )
+ rest_mock.head(
+ f"{TEST_URI}v1/namespaces/some_namespace",
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
Review Comment:
nit: This mock seems redundant because the catalog raises an exception
before verifying the target namespace. No requested change, I suppose you put
it intentionally.
##########
tests/catalog/test_rest.py:
##########
@@ -3167,3 +3167,122 @@ def test_load_table_without_storage_credentials(
)
assert actual.metadata.model_dump() == expected.metadata.model_dump()
assert actual == expected
+
Review Comment:
Can we add an integration test? I've added initial tests in
https://github.com/apache/iceberg-python/pull/3406.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
