Core: Improve view/table detection when replacing a table/view

apache · Nov 9, 2023 · c63be65 · c63be65
1 parent 5754ddb
commit c63be65
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 9 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -55,6 +55,7 @@
 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.catalog.TableCommit;
 import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
 import org.apache.iceberg.exceptions.NoSuchNamespaceException;
 import org.apache.iceberg.exceptions.NoSuchTableException;
 import org.apache.iceberg.exceptions.NoSuchViewException;
@@ -702,6 +703,10 @@ public Transaction createTransaction() {
 
     @Override
     public Transaction replaceTransaction() {
+      if (viewExists(context, ident)) {
+        throw new AlreadyExistsException("View with same name already exists: %s", ident);
+      }
+
       LoadTableResponse response = loadInternal(context, ident, snapshotMode);
       String fullName = fullTableName(ident);
 
@@ -1170,6 +1175,10 @@ public View createOrReplace() {
 
     @Override
     public View replace() {
+      if (tableExists(context, identifier)) {
+        throw new AlreadyExistsException("Table with same name already exists: %s", identifier);
+      }
+
       return replace(loadView());
     }
 

diff --git a/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java b/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java
@@ -23,6 +23,7 @@
 import org.apache.iceberg.BaseMetastoreCatalog;
 import org.apache.iceberg.EnvironmentContext;
 import org.apache.iceberg.Schema;
+import org.apache.iceberg.Transaction;
 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.iceberg.catalog.ViewCatalog;
@@ -182,6 +183,10 @@ private View create(ViewOperations ops) {
     }
 
     private View replace(ViewOperations ops) {
+      if (tableExists(identifier)) {
+        throw new AlreadyExistsException("Table with same name already exists: %s", identifier);
+      }
+
       if (null == ops.current()) {
         throw new NoSuchViewException("View does not exist: %s", identifier);
       }
@@ -230,4 +235,28 @@ private View replace(ViewOperations ops) {
       return new BaseView(ops, ViewUtil.fullViewName(name(), identifier));
     }
   }
+
+  @Override
+  public TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+    return new BaseMetastoreViewCatalogTableBuilder(identifier, schema);
+  }
+
+  /** The purpose of this class is to add view detection when replacing a table */
+  protected class BaseMetastoreViewCatalogTableBuilder extends BaseMetastoreCatalogTableBuilder {
+    private final TableIdentifier identifier;
+
+    public BaseMetastoreViewCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+      super(identifier, schema);
+      this.identifier = identifier;
+    }
+
+    @Override
+    public Transaction replaceTransaction() {
+      if (viewExists(identifier)) {
+        throw new AlreadyExistsException("View with same name already exists: %s", identifier);
+      }
+
+      return super.replaceTransaction();
+    }
+  }
 }
diff --git a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
@@ -35,7 +35,6 @@
 import org.apache.iceberg.catalog.ViewCatalog;
 import org.apache.iceberg.exceptions.AlreadyExistsException;
 import org.apache.iceberg.exceptions.NoSuchNamespaceException;
-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.types.Types;
@@ -396,16 +395,14 @@ public void replaceTableViaTransactionThatAlreadyExistsAsView() {
 
     assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue();
 
-    // replace transaction requires table existence
-    // TODO: replace should check whether the table exists as a view
     assertThatThrownBy(
             () ->
                 tableCatalog()
                     .buildTable(viewIdentifier, SCHEMA)
                     .replaceTransaction()
                     .commitTransaction())
-        .isInstanceOf(NoSuchTableException.class)
-        .hasMessageStartingWith("Table does not exist: ns.view");
+        .isInstanceOf(AlreadyExistsException.class)
+        .hasMessageStartingWith("View with same name already exists: ns.view");
   }
 
   @Test
@@ -459,8 +456,6 @@ public void replaceViewThatAlreadyExistsAsTable() {
 
     assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue();
 
-    // replace view requires the view to exist
-    // TODO: replace should check whether the view exists as a table
     assertThatThrownBy(
             () ->
                 catalog()
@@ -469,8 +464,8 @@ public void replaceViewThatAlreadyExistsAsTable() {
                     .withDefaultNamespace(tableIdentifier.namespace())
                     .withQuery("spark", "select * from ns.tbl")
                     .replace())
-        .isInstanceOf(NoSuchViewException.class)
-        .hasMessageStartingWith("View does not exist: ns.table");
+        .isInstanceOf(AlreadyExistsException.class)
+        .hasMessageContaining("Table with same name already exists: ns.table");
   }
 
   @Test