geruh commented on code in PR #3349:
URL: https://github.com/apache/iceberg-python/pull/3349#discussion_r3215436595
##########
tests/catalog/test_rest.py:
##########
@@ -640,6 +639,54 @@ def test_list_views_200(rest_mock: Mocker) -> None:
assert RestCatalog("rest", uri=TEST_URI,
token=TEST_TOKEN).list_views(namespace) == [("examples", "fooshare")]
+def test_list_views_paginated_200(rest_mock: Mocker) -> None:
Review Comment:
Can we add some tests for an ommitted field and null for the
next_page_token, and ensure the behavior is the same?
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1105,26 +1103,31 @@ def list_views(self, namespace: str | Identifier) ->
list[Identifier]:
return []
namespace_tuple = self._check_valid_namespace_identifier(namespace)
namespace_concat = self._encode_namespace_path(namespace_tuple)
- response = self._session.get(self.url(Endpoints.list_views,
namespace=namespace_concat))
- try:
- response.raise_for_status()
- except HTTPError as exc:
- _handle_non_200_response(exc, {404: NoSuchNamespaceError})
- return [(*view.namespace, view.name) for view in
ListViewsResponse.model_validate_json(response.text).identifiers]
- @retry(**_RETRY_ARGS)
- def load_view(self, identifier: str | Identifier) -> View:
- self._check_endpoint(Capability.V1_LOAD_VIEW)
- response = self._session.get(
- self.url(Endpoints.load_view, prefixed=True,
**self._split_identifier_for_path(identifier, IdentifierKind.VIEW))
- )
- try:
- response.raise_for_status()
- except HTTPError as exc:
- _handle_non_200_response(exc, {404: NoSuchViewError})
+ all_identifiers: list[Identifier] = []
+ page_token: str | None = None
- view_response = ViewResponse.model_validate_json(response.text)
- return self._response_to_view(self.identifier_to_tuple(identifier),
view_response)
+ while True:
+ # Build URL with pagination params
Review Comment:
nit: unnecessary comment with this and below since code is demonstrating
this alr
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1105,26 +1103,31 @@ def list_views(self, namespace: str | Identifier) ->
list[Identifier]:
return []
namespace_tuple = self._check_valid_namespace_identifier(namespace)
namespace_concat = self._encode_namespace_path(namespace_tuple)
- response = self._session.get(self.url(Endpoints.list_views,
namespace=namespace_concat))
- try:
- response.raise_for_status()
- except HTTPError as exc:
- _handle_non_200_response(exc, {404: NoSuchNamespaceError})
- return [(*view.namespace, view.name) for view in
ListViewsResponse.model_validate_json(response.text).identifiers]
- @retry(**_RETRY_ARGS)
- def load_view(self, identifier: str | Identifier) -> View:
- self._check_endpoint(Capability.V1_LOAD_VIEW)
- response = self._session.get(
- self.url(Endpoints.load_view, prefixed=True,
**self._split_identifier_for_path(identifier, IdentifierKind.VIEW))
- )
- try:
- response.raise_for_status()
- except HTTPError as exc:
- _handle_non_200_response(exc, {404: NoSuchViewError})
+ all_identifiers: list[Identifier] = []
+ page_token: str | None = None
- view_response = ViewResponse.model_validate_json(response.text)
- return self._response_to_view(self.identifier_to_tuple(identifier),
view_response)
+ while True:
+ # Build URL with pagination params
+ url = self.url(Endpoints.list_views, namespace=namespace_concat)
+ if page_token:
+ url = f"{url}?pageToken={page_token}"
Review Comment:
This should use a param map instead of string concatenation to pass in
`pageToken`. Especially because the token is defined as an opaque string that
can contain symbols that could be messed up by encoding.
--
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]
