[#314] feat(catalog-lakehouse): support Iceberg table interface #350
Conversation
Code Coverage Report
Files
|
...use/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/ops/IcebergTableOps.java
Outdated
Show resolved
Hide resolved
|
@jerryshao , it's ready to review now. |
|
some works of @yunqing-wei (IcebergCatalog table operations) depends on this pr, @jerryshao please have a review priorly |
jerryshao
changed the title
...use/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/ops/IcebergTableOps.java
Outdated
Show resolved
Hide resolved
| @PathParam("namespace") String namespace, | ||
| @PathParam("table") String table, | ||
| @DefaultValue("all") @QueryParam("snapshots") String snapshots) { | ||
| // todo support snapshots |
There was a problem hiding this comment.
When will you do this snapshot support.
There was a problem hiding this comment.
I try to merge the logic to the CatalogHandlers in upstream, snapshots is a performance feature，does't impact the correctness.
There was a problem hiding this comment.
what's the behavior of the current Iceberg? Does it support snapshot in load table?
...est/java/com/datastrato/graviton/catalog/lakehouse/iceberg/web/rest/IcebergRestTestUtil.java
Show resolved
Hide resolved
|
@yunqing-wei would you please also help to review this, thanks. |
|
+1 |
| verifyCreateTableSucc("rename_foo3"); | ||
| verifyRenameTableFail("rename_foo2", "rename_foo3", 409); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if we fully cover all the return codes in this UT, if not, I would suggest adding more UTs to cover all the scenarios in Iceberg's REST spec.
There was a problem hiding this comment.
I'll check it completely again today
There was a problem hiding this comment.
add more UTs to cover, take testRenameTable for example, covers the rename related logic:
- rename succ, 200
- namespace not exists. 404
- source table not exists. 404
- dest table exits 409
the other return codes are tested in TestIcebergExceptionMapper,
- BadRequest. 400
- Unauthorized 401
- Forbidden. 403
- UnsupportedOperation 406
- ServiceUnavailable. 503
- ServerError 500
AuthenticationTimeout is defined in api-spec, but not defined in Iceberg api and not handled in REST client code. not covered now.
What changes were proposed in this pull request?
add table interface for Iceberg REST server
Why are the changes needed?
Iceberg REST api spec
Fix: #314
Does this PR introduce any user-facing change?
new api
How was this patch tested?
UT