sakshichitnis27 commented on code in PR #18665:
URL: https://github.com/apache/iceberg/pull/18665#discussion_r4392729210
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1165,12 +1166,19 @@ public Transaction createTransaction() {
@Override
public Transaction replaceTransaction() {
Endpoint.check(endpoints, Endpoint.V1_UPDATE_TABLE);
- if (viewExists(context, ident)) {
- throw new AlreadyExistsException("View with same name already exists:
%s", ident);
- }
Review Comment:
Thanks for taking a look @sungwy, and happy to go whichever way you and
@nastra prefer.
I tried removing the check on a local branch. The
`org.apache.iceberg.rest.*` tests in `iceberg-core` pass except
`replaceTableViaTransactionThatAlreadyExistsAsView` in `TestRESTViewCatalog`
(and the `AssumedViewSupport` variant). With the check removed, a plain
`REPLACE TABLE` on a name that is a view fails with `NoSuchTableException` (the
table load returns 404) instead of `AlreadyExistsException`. For
`createOrReplaceTransaction()` it still ends up as `AlreadyExistsException`,
because the create falls back and the server rejects it.
So removing the check seems fine to me if you're OK with that error change
for the plain replace case. I can then update that one test in
`TestRESTViewCatalog` to expect `NoSuchTableException`. Keeping the check only
for the "table not found" case, as in this PR, keeps the current exception for
that case. Let me know which you'd prefer and I'll update the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
