You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "spr0els (via GitHub)" <gi...@apache.org> on 2026/04/09 13:26:24 UTC
[PR] feat: add load_view to REST catalog [iceberg-python]
spr0els opened a new pull request, #3224:
URL: https://github.com/apache/iceberg-python/pull/3224
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
This is part of #818 and implements the `load_view` method for REST catalogs.
## Are these changes tested?
Unit tests are added for:
- successful loading of a view
- thrown 404 error when trying to load a non existing view
## Are there any user-facing changes?
This adds the `load_view` method for REST catalogs
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3185347605
##########
tests/integration/test_rest_catalog.py:
##########
@@ -62,3 +67,47 @@ def test_create_namespace_if_already_existing(catalog: RestCatalog) -> None:
catalog.create_namespace_if_not_exists(TEST_NAMESPACE_IDENTIFIER)
assert catalog.namespace_exists(TEST_NAMESPACE_IDENTIFIER)
+
+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [lf("session_catalog")])
+def test_load_view(catalog: RestCatalog, table_schema_nested: Schema, database_name: str, view_name: str) -> None:
+ identifier = (database_name, view_name)
+ if not catalog.namespace_exists(database_name):
+ catalog.create_namespace(database_name)
+
+ view_version = ViewVersion(
+ version_id=1,
+ schema_id=1,
+ timestamp_ms=int(time.time() * 1000),
+ summary={},
+ representations=[
+ SQLViewRepresentation(
+ type="sql",
+ sql="SELECT 1 as some_col",
+ dialect="spark",
+ )
+ ],
+ default_namespace=["default"],
+ )
+ view = catalog.create_view(identifier, table_schema_nested, view_version=view_version)
+ loaded_view = catalog.load_view(identifier)
+ assert view.name() == loaded_view.name()
+ assert view.metadata == loaded_view.metadata
Review Comment:
We could verify the entire object instead:
```suggestion
assert view == loaded_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: 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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#issuecomment-4262957889
I think we just need a rebase here, and I agree with @gabeiglio on adding an integ test if tck has the view support.
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "spr0els (via GitHub)" <gi...@apache.org>.
spr0els commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3148190676
##########
tests/catalog/test_rest.py:
##########
@@ -1449,6 +1450,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/non_existent_view",
+ json={
+ "error": {
+ "message": "View does not exist: examples.non_existent_view in warehouse 8bcb0838-50fc-472d-9ddb-8feb89ef5f1e",
+ "type": "NoSuchNamespaceErrorException",
Review Comment:
Good catch. I agree, this should be a `NoSuchViewException`.
There are a two tests for tables in the same file that also return the wrong error type. These should return `NoSuchTableException`.
https://github.com/apache/iceberg-python/blob/b67b7241690e05413f3cfea69287f897b0b2410e/tests/catalog/test_rest.py#L1157-L1160
https://github.com/apache/iceberg/blob/8d0508308cd068778897c06d3a802740ed4305a6/open-api/rest-catalog-open-api.yaml#L1009-L1018
https://github.com/apache/iceberg-python/blob/b67b7241690e05413f3cfea69287f897b0b2410e/tests/catalog/test_rest.py#L1218-L1221
https://github.com/apache/iceberg/blob/8d0508308cd068778897c06d3a802740ed4305a6/open-api/rest-catalog-open-api.yaml#L1170-L1179
Should I fix all of these in one go or create a issue for the table tests?
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3067919322
##########
tests/catalog/test_rest.py:
##########
@@ -1416,6 +1417,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
Review Comment:
Would also add a test that should return a view does not exists when trying to load a table with loadView
##########
pyiceberg/catalog/__init__.py:
##########
@@ -644,6 +644,23 @@ def list_views(self, namespace: str | Identifier) -> list[Identifier]:
NoSuchNamespaceError: If a namespace with the given name does not exist.
"""
+ @abstractmethod
+ def load_view(self, identifier: str | Identifier) -> View:
+ """Load the view's metadata and returns the view instance.
+
+ You can also use this method to check for view existence using 'try catalog.load_view() except NoSuchViewError'.
+ Note: This method doesn't scan data stored in the view.
Review Comment:
Nit: I would say we remove these.
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "spr0els (via GitHub)" <gi...@apache.org>.
spr0els commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3086776187
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1099,6 +1102,21 @@ def list_views(self, namespace: str | Identifier) -> list[Identifier]:
_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)),
+ params={},
Review Comment:
Removed the empty params
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3083715465
##########
pyiceberg/catalog/__init__.py:
##########
@@ -644,6 +644,23 @@ def list_views(self, namespace: str | Identifier) -> list[Identifier]:
NoSuchNamespaceError: If a namespace with the given name does not exist.
"""
+ @abstractmethod
+ def load_view(self, identifier: str | Identifier) -> View:
+ """Load the view's metadata and returns the view instance.
+
+ You can also use this method to check for view existence using 'try catalog.load_view() except NoSuchViewError'.
+ Note: This method doesn't scan data stored in the view.
Review Comment:
+1 this is simply just load a view it doesn't have to follow the convention of load_table.
ref: https://github.com/apache/iceberg/blob/53374bde66955885de6fca0ea59b946346ec2d89/api/src/main/java/org/apache/iceberg/catalog/ViewCatalog.java#L49
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -1099,6 +1102,21 @@ def list_views(self, namespace: str | Identifier) -> list[Identifier]:
_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)),
+ params={},
Review Comment:
nitty: we don't need to pass in the empty params
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "spr0els (via GitHub)" <gi...@apache.org>.
spr0els commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3086779400
##########
pyiceberg/catalog/__init__.py:
##########
@@ -644,6 +644,23 @@ def list_views(self, namespace: str | Identifier) -> list[Identifier]:
NoSuchNamespaceError: If a namespace with the given name does not exist.
"""
+ @abstractmethod
+ def load_view(self, identifier: str | Identifier) -> View:
+ """Load the view's metadata and returns the view instance.
+
+ You can also use this method to check for view existence using 'try catalog.load_view() except NoSuchViewError'.
+ Note: This method doesn't scan data stored in the view.
Review Comment:
Updated the docstring
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "spr0els (via GitHub)" <gi...@apache.org>.
spr0els commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3071674422
##########
tests/catalog/test_rest.py:
##########
@@ -1416,6 +1417,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
Review Comment:
Correct me if I'm wrong, but isn't this case already covered with the `test_load_view_404` test?
If no view with the same identifier as the table identifier is found in the warehouse, this would be the same case as loading a non existent 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: 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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3166620355
##########
tests/catalog/test_rest.py:
##########
@@ -1449,6 +1450,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/non_existent_view",
+ json={
+ "error": {
+ "message": "View does not exist: examples.non_existent_view in warehouse 8bcb0838-50fc-472d-9ddb-8feb89ef5f1e",
+ "type": "NoSuchNamespaceErrorException",
Review Comment:
I think we can do them in a different PR to keep this focused on load_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: 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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3095645092
##########
tests/catalog/test_rest.py:
##########
@@ -1416,6 +1417,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
Review Comment:
This is implementation dependent on the server, so this is an edge case to return the correct exception
But the test is better placed in integration tests rather than mocking
Would be cool to also add integration tests to the test_catalog.py
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "spr0els (via GitHub)" <gi...@apache.org>.
spr0els commented on PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#issuecomment-4305625326
I added integration tests to `tests/integration/test_rest_catalog.py` because most catalogs don't have the methods for view interaction implemented yet. Putting the tests in `test_catalog.py` would cause too much error handling for most catalogs at the moment imho.
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#discussion_r3141198485
##########
tests/catalog/test_rest.py:
##########
@@ -1449,6 +1450,38 @@ def test_create_view_409(
assert "View already exists" in str(e.value)
+def test_load_view_200(rest_mock: Mocker, example_view_metadata_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/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.load_view(("fokko", "view"))
+ expected = View(identifier=("fokko", "view"), metadata=ViewMetadata(**example_view_metadata_rest_json["metadata"]))
+ assert actual == expected
+
+
+def test_load_view_404(rest_mock: Mocker) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/views/non_existent_view",
+ json={
+ "error": {
+ "message": "View does not exist: examples.non_existent_view in warehouse 8bcb0838-50fc-472d-9ddb-8feb89ef5f1e",
+ "type": "NoSuchNamespaceErrorException",
Review Comment:
Why is this NoSuchNamespaceErrorException instead of NoSuchViewException?
https://github.com/apache/iceberg/blob/f29a182eccc9287764a511767a79f741eb4135af/open-api/rest-catalog-open-api.yaml#L1648-L1657
```yml
404:
description:
Not Found - NoSuchViewException, view to load does not exist
content:
application/json:
schema:
$ref: '#/components/schemas/ErrorModel'
examples:
ViewToLoadDoesNotExist:
$ref: '#/components/examples/NoSuchViewError'
```
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh merged PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224
--
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
Re: [PR] feat: add load_view to REST catalog [iceberg-python]
Posted by "gabeiglio (via GitHub)" <gi...@apache.org>.
gabeiglio commented on PR #3224:
URL: https://github.com/apache/iceberg-python/pull/3224#issuecomment-4386929891
thanks! lgtm cc: @geruh for when you can double check 😉
--
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