Address Gabor feedback

Signed-off-by: Hongyue Zhang <[email protected]>
apache · Feb 13, 2025 · dbee263 · dbee263
1 parent 08efd36
commit dbee263
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 30 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/catalog/Catalog.java b/api/src/main/java/org/apache/iceberg/catalog/Catalog.java
@@ -360,7 +360,7 @@ default Table registerTable(TableIdentifier identifier, String metadataFileLocat
    */
   default Table registerTable(
       TableIdentifier identifier, String metadataFileLocation, boolean overwrite) {
-    throw new UnsupportedOperationException("Registering tables with overwrite is not supported");
+    throw new UnsupportedOperationException("Registering tables is not supported");
   }
 
   /**

diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
@@ -82,13 +82,13 @@ public Table registerTable(
     // Throw an exception if this table already exists in the catalog.
     if (tableExists(identifier) && !overwrite) {
       throw new AlreadyExistsException("Table already exists: %s", identifier);
-    } else {
-      TableOperations ops = newTableOps(identifier);
-      InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
-      TableMetadata currentMetadata = tableExists(identifier) ? ops.current() : null;
-      ops.commit(currentMetadata, TableMetadataParser.read(ops.io(), metadataFile));
-      return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
     }
+
+    TableOperations ops = newTableOps(identifier);
+    InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+    TableMetadata currentMetadata = tableExists(identifier) ? ops.current() : null;
+    ops.commit(currentMetadata, TableMetadataParser.read(ops.io(), metadataFile));
+    return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
   }
 
   @Override

diff --git a/core/src/main/java/org/apache/iceberg/CachingCatalog.java b/core/src/main/java/org/apache/iceberg/CachingCatalog.java
@@ -190,13 +190,6 @@ public void invalidateTable(TableIdentifier ident) {
     tableCache.invalidateAll(metadataTableIdentifiers(canonicalized));
   }
 
-  @Override
-  public Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
-    Table table = catalog.registerTable(identifier, metadataFileLocation);
-    invalidateTable(identifier);
-    return table;
-  }
-
   @Override
   public Table registerTable(
       TableIdentifier identifier, String metadataFileLocation, boolean overwrite) {

diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
@@ -2890,14 +2890,17 @@ public void testRegisterAndOverwriteExistingTable() {
     catalog.createTable(identifier, SCHEMA);
     Table table = catalog.loadTable(identifier);
     TableOperations ops = ((BaseTable) table).operations();
-    assertThat(table.spec().isPartitioned()).isFalse();
+    String unpartitionedMetadataLocation = ops.current().metadataFileLocation();
+
     // update table spec
     table.updateSpec().addField(bucket("id", 16)).commit();
-    String newMetadataLocation = ops.refresh().metadataFileLocation();
+    assertThat(table.spec().isPartitioned()).isTrue();
+
     // register and overwrite
-    catalog.registerTable(identifier, newMetadataLocation, true);
+    catalog.registerTable(identifier, unpartitionedMetadataLocation, true);
+
     table.refresh();
-    assertThat(table.spec().isPartitioned()).isTrue();
+    assertThat(table.spec().isPartitioned()).isFalse();
     assertThat(catalog.dropTable(identifier)).isTrue();
   }
 

diff --git a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
@@ -672,19 +672,19 @@ public void testRegisterExistingTable() throws IOException {
     HadoopCatalog catalog = hadoopCatalog();
     catalog.createTable(identifier, SCHEMA);
     Table registeringTable = catalog.loadTable(identifier);
-    assertThat(registeringTable.spec().isPartitioned()).isFalse();
     TableOperations ops = ((HasTableOperations) registeringTable).operations();
-    String oldMetadataLocation = ops.current().metadataFileLocation();
+    String unpartitionedMetadataLocation = ops.current().metadataFileLocation();
     // update table spec
     registeringTable.updateSpec().addField(bucket("id", 16)).commit();
-    String newMetadataLocation = ops.refresh().metadataFileLocation();
+    assertThat(registeringTable.spec().isPartitioned()).isTrue();
     // register w/o overwrite
-    assertThatThrownBy(() -> catalog.registerTable(identifier, oldMetadataLocation, false))
+    assertThatThrownBy(
+            () -> catalog.registerTable(identifier, unpartitionedMetadataLocation, false))
         .isInstanceOf(AlreadyExistsException.class)
         .hasMessage("Table already exists: a.t1");
     // register with overwrite
-    catalog.registerTable(identifier, newMetadataLocation, true);
-    assertThat(catalog.loadTable(identifier).spec().isPartitioned()).isTrue();
+    catalog.registerTable(identifier, unpartitionedMetadataLocation, true);
+    assertThat(catalog.loadTable(identifier).spec().isPartitioned()).isFalse();
     assertThat(catalog.dropTable(identifier)).isTrue();
   }
 }
diff --git a/dell/src/test/java/org/apache/iceberg/dell/ecs/TestEcsCatalog.java b/dell/src/test/java/org/apache/iceberg/dell/ecs/TestEcsCatalog.java
@@ -195,19 +195,19 @@ public void testRegisterExistingTable() {
     TableIdentifier identifier = TableIdentifier.of("a", "t1");
     ecsCatalog.createTable(identifier, SCHEMA);
     Table registeringTable = ecsCatalog.loadTable(identifier);
-    assertThat(registeringTable.spec().isPartitioned()).isFalse();
     TableOperations ops = ((HasTableOperations) registeringTable).operations();
-    String oldMetadataLocation = ops.current().metadataFileLocation();
+    String unpartitionedMetadataLocation = ops.current().metadataFileLocation();
     // update table spec
     registeringTable.updateSpec().addField(bucket("id", 16)).commit();
-    String newMetadataLocation = ops.refresh().metadataFileLocation();
+    assertThat(registeringTable.spec().isPartitioned()).isTrue();
     // register w/o overwrite
-    assertThatThrownBy(() -> ecsCatalog.registerTable(identifier, oldMetadataLocation, false))
+    assertThatThrownBy(
+            () -> ecsCatalog.registerTable(identifier, unpartitionedMetadataLocation, false))
         .isInstanceOf(AlreadyExistsException.class)
         .hasMessage("Table already exists: a.t1");
     // register with overwrite
-    ecsCatalog.registerTable(identifier, newMetadataLocation, true);
-    assertThat(ecsCatalog.loadTable(identifier).spec().isPartitioned()).isTrue();
+    ecsCatalog.registerTable(identifier, unpartitionedMetadataLocation, true);
+    assertThat(ecsCatalog.loadTable(identifier).spec().isPartitioned()).isFalse();
     assertThat(ecsCatalog.dropTable(identifier, true)).isTrue();
   }
 }