geruh commented on code in PR #3288:
URL: https://github.com/apache/iceberg-python/pull/3288#discussion_r3150384296
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1312,6 +1319,27 @@ def view_exists(self, identifier: str | Identifier) ->
bool:
return False
+ @retry(**_RETRY_ARGS)
+ def register_view(self, identifier: str | Identifier, metadata_location:
str) -> View:
+ self._check_endpoint(Capability.V1_REGISTER_VIEW)
+ namespace_and_view = self._split_identifier_for_path(identifier)
Review Comment:
`self._split_identifier_for_path(identifier, IdentifierKind.VIEW)` here?
Then you can do:
```
namespace_and_view = self._split_identifier_for_path(identifier,
IdentifierKind.VIEW)
request = RegisterViewRequest(name=namespace_and_view["view"],
metadata_location=metadata_location)
```
##########
tests/catalog/test_rest.py:
##########
@@ -2122,6 +2123,47 @@ def test_table_identifier_in_commit_table_request(
)
+def test_register_view_200(rest_mock: Mocker, example_view_metadata_rest_json:
dict[str, Any]) -> None:
+ rest_mock.post(
+ f"{TEST_URI}v1/namespaces/default/register-view",
+ json=example_view_metadata_rest_json,
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ actual = catalog.register_view(
+ identifier=("default", "registered_view"),
metadata_location="s3://warehouse/database/view/metadata.json"
+ )
+ expected = View(
+ identifier=("default", "registered_view"),
+ metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]),
+ )
+ assert actual.metadata.model_dump() == expected.metadata.model_dump()
Review Comment:
You can follow the test from `test_create_view_200` and just do a `assert
actual == expected` since the `__eq__` already passes
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1312,6 +1319,27 @@ def view_exists(self, identifier: str | Identifier) ->
bool:
return False
+ @retry(**_RETRY_ARGS)
+ def register_view(self, identifier: str | Identifier, metadata_location:
str) -> View:
+ self._check_endpoint(Capability.V1_REGISTER_VIEW)
+ namespace_and_view = self._split_identifier_for_path(identifier)
+ request = RegisterViewRequest(
+ name=self._identifier_to_validated_tuple(identifier)[-1],
+ metadata_location=metadata_location,
+ )
+ serialized_json = request.model_dump_json().encode(UTF8)
+ response = self._session.post(
+ self.url(Endpoints.register_view,
namespace=namespace_and_view["namespace"]),
+ data=serialized_json,
+ )
+ try:
+ response.raise_for_status()
+ except HTTPError as exc:
+ _handle_non_200_response(exc, {409: ViewAlreadyExistsError})
Review Comment:
I agree with this, this behavior is seen in the java impl:
https://github.com/apache/iceberg/blob/b0f022ff29945fe70f51485a1f1cda3d35eed8ca/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java#L310-L316
This is also what the rest spec defines
https://github.com/apache/iceberg/blob/b0f022ff29945fe70f51485a1f1cda3d35eed8ca/open-api/rest-catalog-open-api.yaml#L1927-L1935
Following your implementation you're making a register view requests w/o the
existence checks which makes it hard to tell if the 409 came from table or view.
--
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]
