Core: Implement register view for REST catalog #14870
Conversation
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
Outdated
Show resolved
Hide resolved
ajantha-bhat
force-pushed
the
view-rest
branch
from
18c1b4c to
3569697
Compare
ajantha-bhat
force-pushed
the
view-rest
branch
3 times, most recently
from
d9b60d5 to
5da028a
Compare
ajantha-bhat
force-pushed
the
view-rest
branch
from
5da028a to
cf14eba
Compare
core/src/main/java/org/apache/iceberg/rest/requests/RegisterViewRequest.java
Show resolved
Hide resolved
ajantha-bhat
force-pushed
the
view-rest
branch
from
cf14eba to
df65166
Compare
ajantha-bhat
force-pushed
the
view-rest
branch
from
df65166 to
88359cf
Compare
| assertThat(catalog.dropView(identifier)).isTrue(); | ||
| assertThat(catalog.viewExists(identifier)).as("View must not exist").isFalse(); | ||
|
|
||
| // view metadata should still exist after dropping the view as gc is disabled |
There was a problem hiding this comment.
For the REST catalog, it won't be BaseViewOperations, it is RESTViewOperations and doesn't expose fileIO. So, removed the check.
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.
There was a problem hiding this comment.
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.
|
Rebasing the PR due to conflict in |
Implement view registration support in REST catalog using the RegisterViewRequest. This adds: - registerView method to ViewSessionCatalog and BaseViewSessionCatalog - REST endpoint handling in CatalogHandlers - Client-side implementation in RESTCatalog and RESTSessionCatalog - Resource path support for register-view endpoint - Tests for the complete registration flow
ajantha-bhat
force-pushed
the
view-rest
branch
from
88359cf to
f0eaf7c
Compare
nastra
changed the title
| checkViewIdentifierIsValid(ident); | ||
|
|
||
| Preconditions.checkArgument( | ||
| metadataFileLocation != null && !metadataFileLocation.isEmpty(), |
There was a problem hiding this comment.
nit: We could use Strings.isNullOrEmpty method. Feel free to ignore this comment. I know this condition is consistent with registerTable.
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @ajantha-bhat! had a minor comment on one of the test changes but other than that it looks good to me
| assertThat(catalog.dropView(identifier)).isTrue(); | ||
| assertThat(catalog.viewExists(identifier)).as("View must not exist").isFalse(); | ||
|
|
||
| // view metadata should still exist after dropping the view as gc is disabled |
There was a problem hiding this comment.
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.
|
Thanks everyone for the reviews. I have addressed the nits and PR is ready. |
5 participants
Implement view registration support in REST catalog using
the RegisterViewRequest. This adds: