Core: Add View support for REST catalog #7913
Conversation
16eb260 to
d2d5be6
Compare
core/src/main/java/org/apache/iceberg/rest/requests/UpdateViewRequest.java
Outdated
Show resolved
Hide resolved
d2d5be6 to
1c9df82
Compare
045f67d to
dd6eac0
Compare
75dfd77 to
b62b93d
Compare
b62b93d to
1396eb1
Compare
50a65bb to
59bde7f
Compare
59bde7f to
ac36663
Compare
3d01bb9 to
626dedc
Compare
9905345 to
9fb62ee
Compare
9fb62ee to
30753ba
Compare
|
|
||
| private int addVersionInternal(ViewVersion version) { | ||
| int newVersionId = reuseOrCreateNewViewVersionId(version); | ||
| private int addVersionInternal(ViewVersion newVersion) { |
There was a problem hiding this comment.
I've renamed this parameter to make the code slightly easier to understand, because we have a few cases in this method where we re-assign either the schema id or the view version id
| .create()) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Invalid view version: Cannot add multiple queries for dialect trino"); | ||
| .isInstanceOf(Exception.class) |
There was a problem hiding this comment.
Minor: I think I pointed this out before and may have forgotten the answer, but shouldn't this be a consistent exception class every time?
There was a problem hiding this comment.
I remember I replied to this but somehow can't find where it was.
The issue with the REST version is unfortunately that it fails with org.apache.iceberg.exceptions.BadRequestException: Malformed request: Invalid view version: Cannot add multiple queries for dialect trino because we're re-applying all changes to the Builder in CatalogHandlers.createView(...):
iceberg/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
Lines 424 to 429 in f076b4d
That IAE is then converted to a BadRequestException in
iceberg/core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
Lines 204 to 205 in ace0b13
So I think what we could do is to have
if (IllegalArgumentException.class.getSimpleName().equals(error.type())) {
throw new IllegalArgumentException(error.message());
}
in the DefaultErrorHandler. Thoughts?
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
Outdated
Show resolved
Hide resolved
rdblue
left a comment
There was a problem hiding this comment.
I think this is nearly ready but the concurrent test case actually refreshes correctly and doesn't test the concurrent case with server-side retry and ID reassignment. I made a suggestion in the comment that I think will fix the test issue.
This will probably also require a flag in ViewCatalogTests for when server-side retry is possible like in table tests. If we're testing it correctly, then the InMemoryCatalog will fail.
|
Thanks @nastra! Great to have this in. |
(cherry picked from commit f19643a)
No description provided.