Add rename_view to REST Catalog#2149
Conversation
|
@Fokko @kevinjqliu mind taking a look when you can? thanks! |
rambleraptor
force-pushed
the
rename_view
branch
from
f4e2a61 to
756ee0b
Compare
rambleraptor
force-pushed
the
rename_view
branch
from
756ee0b to
8ad2e55
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
rambleraptor
force-pushed
the
rename_view
branch
from
8ad2e55 to
9188ced
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
rambleraptor
force-pushed
the
rename_view
branch
from
c8996c2 to
7f39e33
Compare
|
@geruh looks like you've been going through the View PRs. Mind taking a look at this one? Follows the others near-identically. |
|
@ebyhr can you take a look? You've been doing a lot of work around views lately. |
|
@kevinjqliu @Fokko @geruh mind taking a look? This will help with all of the View work that's been done. |
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_rest_rename_view( |
There was a problem hiding this comment.
Would it be possible to also include a cross namespace rename test for completeness?
|
|
||
| @abstractmethod | ||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: | ||
| """Rename a fully classified view name. |
There was a problem hiding this comment.
minor: fully qualified instead of fully classified?
| raise NotImplementedError | ||
|
|
||
| @override | ||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: |
There was a problem hiding this comment.
It seems the override decorator for load_namespace_properties was accidentally lost?
| if hasattr(self, "engine"): | ||
| self.engine.dispose() | ||
|
|
||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: |
There was a problem hiding this comment.
Is this missing the override decorator?
| _handle_non_200_response(exc, {404: NoSuchViewError}) | ||
|
|
||
| @retry(**_RETRY_ARGS) | ||
| def rename_view(self, from_identifier: str | Identifier, to_identifier: str | Identifier) -> None: |
There was a problem hiding this comment.
Is this missing an override decorator?
5 participants
Rationale for this change
As part of #818, this adds
rename_viewsupport to the Iceberg REST Catalog.The REST Catalog spec specifies that both
rename_viewandrename_tabledo not return anything. Currently,rename_tablereturns the table. I'm happy to change that API if we want, but it would technically be a breaking change.Are these changes tested?
Added unit tests.
Are there any user-facing changes?
rename_viewsupport to Iceberg REST Catalog.