-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: Implement register view for REST catalog #14870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,6 @@ | |
| import org.apache.iceberg.exceptions.NoSuchTableException; | ||
| import org.apache.iceberg.exceptions.NoSuchViewException; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; | ||
| import org.apache.iceberg.rest.RESTCatalog; | ||
| import org.apache.iceberg.types.Types; | ||
| import org.apache.iceberg.util.LocationUtil; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
@@ -2013,10 +2012,6 @@ public void registerTableThatAlreadyExistsAsView() { | |
| public void registerView() { | ||
| C catalog = catalog(); | ||
|
|
||
| assumeThat(catalog) | ||
| .as("Registering a view is not yet supported for the REST catalog") | ||
| .isNotInstanceOf(RESTCatalog.class); | ||
|
|
||
| TableIdentifier identifier = TableIdentifier.of("ns", "view"); | ||
|
|
||
| if (requiresNamespaceCreate()) { | ||
|
|
@@ -2040,7 +2035,9 @@ public void registerView() { | |
| assertThat(catalog.viewExists(identifier)).as("View must not exist").isFalse(); | ||
|
|
||
| // view metadata should still exist after dropping the view as gc is disabled | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the REST catalog, it won't be While Registering the view, if the file doesn't exist, It will throw the exception anyways and testcase will fail in that case. Now it is passing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: We may still want to just conditionally perform this assertion if it's a BaseViewOperations that the client doesn't just drop it. You're right that we'd expect the subsequent register to fail if the metadata file doesn't exist but if someone wants to run this test suite against some arbitrary catalog that has a bug on cleanup or maybe there's a bug in that implementation where register doesn't really check the metadata file, it'd be nice to have a clearer failure here rather than proceeding. Keeping the assertion in that case is a minor additional check that allows us to more clearly catch issues like that. |
||
| assertThat(((BaseViewOperations) ops).io().newInputFile(metadataLocation).exists()).isTrue(); | ||
| if (ops instanceof BaseViewOperations) { | ||
| assertThat(((BaseViewOperations) ops).io().newInputFile(metadataLocation).exists()).isTrue(); | ||
| } | ||
|
|
||
| View registeredView = catalog.registerView(identifier, metadataLocation); | ||
|
|
||
|
|
@@ -2085,10 +2082,6 @@ public void registerView() { | |
| public void registerExistingView() { | ||
| C catalog = catalog(); | ||
|
|
||
| assumeThat(catalog) | ||
| .as("Registering a view is not yet supported for the REST catalog") | ||
| .isNotInstanceOf(RESTCatalog.class); | ||
|
|
||
| TableIdentifier identifier = TableIdentifier.of("ns", "view"); | ||
|
|
||
| if (requiresNamespaceCreate()) { | ||
|
|
@@ -2117,10 +2110,6 @@ public void registerExistingView() { | |
| public void registerViewThatAlreadyExistsAsTable() { | ||
| C catalog = catalog(); | ||
|
|
||
| assumeThat(catalog) | ||
| .as("Registering a view is not yet supported for the REST catalog") | ||
| .isNotInstanceOf(RESTCatalog.class); | ||
|
|
||
| TableIdentifier identifier = TableIdentifier.of("ns", "view"); | ||
|
|
||
| if (requiresNamespaceCreate()) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.