diff --git a/aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java b/aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java index 1c21f0ba3117..f6fbfa55cfc2 100644 --- a/aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java +++ b/aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java @@ -369,7 +369,11 @@ public void testRegisterTable() { Assertions.assertThat(catalog.dropTable(identifier, false)).isTrue(); TableOperations ops = ((HasTableOperations) registeringTable).operations(); String metadataLocation = ((DynamoDbTableOperations) ops).currentMetadataLocation(); - Assertions.assertThat(catalog.registerTable(identifier, metadataLocation)).isNotNull(); + Table registeredTable = catalog.registerTable(identifier, metadataLocation); + Assertions.assertThat(registeredTable).isNotNull(); + String expectedMetadataLocation = + ((HasTableOperations) registeredTable).operations().current().metadataFileLocation(); + Assertions.assertThat(metadataLocation).isEqualTo(expectedMetadataLocation); Assertions.assertThat(catalog.loadTable(identifier)).isNotNull(); Assertions.assertThat(catalog.dropTable(identifier, true)).isTrue(); Assertions.assertThat(catalog.dropNamespace(namespace)).isTrue(); diff --git a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java index 051d5bff8bf3..db5457f92235 100644 --- a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java +++ b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java @@ -536,7 +536,11 @@ public void testRegisterTable() { Table table = glueCatalog.loadTable(identifier); String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation(); Assertions.assertThat(glueCatalog.dropTable(identifier, false)).isTrue(); - Assertions.assertThat(glueCatalog.registerTable(identifier, metadataLocation)).isNotNull(); + Table registeredTable = glueCatalog.registerTable(identifier, metadataLocation); + Assertions.assertThat(registeredTable).isNotNull(); + String expectedMetadataLocation = + ((BaseTable) table).operations().current().metadataFileLocation(); + Assertions.assertThat(metadataLocation).isEqualTo(expectedMetadataLocation); Assertions.assertThat(glueCatalog.loadTable(identifier)).isNotNull(); Assertions.assertThat(glueCatalog.dropTable(identifier, true)).isTrue(); Assertions.assertThat(glueCatalog.dropNamespace(Namespace.of(namespace))).isTrue(); diff --git a/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java b/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java index 0fa4f8f0b1a7..e38aa0a86711 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java +++ b/aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java @@ -102,7 +102,8 @@ protected void doRefresh() { @Override protected void doCommit(TableMetadata base, TableMetadata metadata) { - String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); CommitStatus commitStatus = CommitStatus.FAILURE; Map tableKey = DynamoDbCatalog.tablePrimaryKey(tableIdentifier); try { diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java index ad34ce24c55b..84887cf25302 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java +++ b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java @@ -148,7 +148,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) { try { glueTempTableCreated = createGlueTempTableIfNecessary(base, metadata.location()); - newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); lock(newMetadataLocation); Table glueTable = getGlueTable(); checkMetadataLocation(glueTable, base); diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java index 1cbc6608e575..2fccef5a0ab3 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java @@ -154,6 +154,12 @@ protected void disableRefresh() { this.shouldRefresh = false; } + protected String writeNewMetadataIfRequired(boolean newTable, TableMetadata metadata) { + return newTable && metadata.metadataFileLocation() != null + ? metadata.metadataFileLocation() + : writeNewMetadata(metadata, currentVersion() + 1); + } + protected String writeNewMetadata(TableMetadata metadata, int newVersion) { String newTableMetadataFilePath = newTableMetadataFilePath(metadata, newVersion); OutputFile newMetadataLocation = io().newOutputFile(newTableMetadataFilePath); diff --git a/core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java b/core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java index 8e07aa594ac0..cdc6be6c70a4 100644 --- a/core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java +++ b/core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java @@ -102,7 +102,8 @@ public void doRefresh() { @Override public void doCommit(TableMetadata base, TableMetadata metadata) { - String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); try { Map table = getTable(); diff --git a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java index 472824fa5e0d..268391bf436a 100644 --- a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java +++ b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java @@ -782,6 +782,9 @@ public void testRegisterTable() { Table registeredTable = catalog.registerTable(identifier, metadataLocation); Assertions.assertThat(registeredTable).isNotNull(); TestHelpers.assertSerializedAndLoadedMetadata(registeringTable, registeredTable); + String expectedMetadataLocation = + ((HasTableOperations) registeredTable).operations().current().metadataFileLocation(); + Assertions.assertThat(metadataLocation).isEqualTo(expectedMetadataLocation); Assertions.assertThat(catalog.loadTable(identifier)).isNotNull(); Assertions.assertThat(catalog.dropTable(identifier)).isTrue(); } diff --git a/dell/src/main/java/org/apache/iceberg/dell/ecs/EcsTableOperations.java b/dell/src/main/java/org/apache/iceberg/dell/ecs/EcsTableOperations.java index 9f2c24ac3e60..d7467eff4517 100644 --- a/dell/src/main/java/org/apache/iceberg/dell/ecs/EcsTableOperations.java +++ b/dell/src/main/java/org/apache/iceberg/dell/ecs/EcsTableOperations.java @@ -87,7 +87,8 @@ protected void doRefresh() { @Override protected void doCommit(TableMetadata base, TableMetadata metadata) { - String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); if (base == null) { // create a new table, the metadataKey should be absent if (!catalog.putNewProperties(tableObject, buildProperties(newMetadataLocation))) { 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 index 8c667f6898c6..1b26d2317588 100644 --- a/dell/src/test/java/org/apache/iceberg/dell/ecs/TestEcsCatalog.java +++ b/dell/src/test/java/org/apache/iceberg/dell/ecs/TestEcsCatalog.java @@ -190,7 +190,11 @@ public void testRegisterTable() { ecsCatalog.dropTable(identifier, false); TableOperations ops = ((HasTableOperations) registeringTable).operations(); String metadataLocation = ((EcsTableOperations) ops).currentMetadataLocation(); - Assertions.assertThat(ecsCatalog.registerTable(identifier, metadataLocation)).isNotNull(); + Table registeredTable = ecsCatalog.registerTable(identifier, metadataLocation); + Assertions.assertThat(registeredTable).isNotNull(); + String expectedMetadataLocation = + ((HasTableOperations) registeredTable).operations().current().metadataFileLocation(); + Assertions.assertThat(metadataLocation).isEqualTo(expectedMetadataLocation); Assertions.assertThat(ecsCatalog.loadTable(identifier)).isNotNull(); Assertions.assertThat(ecsCatalog.dropTable(identifier, true)).isTrue(); } diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index e5321d2ab518..ae3aea6d09c3 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -268,10 +268,8 @@ protected void doRefresh() { @SuppressWarnings("checkstyle:CyclomaticComplexity") @Override protected void doCommit(TableMetadata base, TableMetadata metadata) { - String newMetadataLocation = - base == null && metadata.metadataFileLocation() != null - ? metadata.metadataFileLocation() - : writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); boolean hiveEngineEnabled = hiveEngineEnabled(metadata, conf); boolean keepHiveStats = conf.getBoolean(ConfigProperties.KEEP_HIVE_STATS, false); @@ -296,7 +294,7 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) { if (tbl != null) { // If we try to create the table but the metadata location is already set, then we had a // concurrent commit - if (base == null + if (newTable && tbl.getParameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP) != null) { throw new AlreadyExistsException("Table already exists: %s.%s", database, tableName); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index a9cae2b28b60..b3735006a34c 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -28,6 +28,7 @@ import static org.apache.iceberg.TableProperties.DEFAULT_SORT_ORDER; import static org.apache.iceberg.expressions.Expressions.bucket; import static org.apache.iceberg.types.Types.NestedField.required; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -49,6 +50,7 @@ import org.apache.iceberg.DataFile; import org.apache.iceberg.DataFiles; import org.apache.iceberg.FileFormat; +import org.apache.iceberg.HasTableOperations; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionSpecParser; import org.apache.iceberg.Schema; @@ -60,6 +62,7 @@ import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableOperations; import org.apache.iceberg.TableProperties; +import org.apache.iceberg.TestHelpers; import org.apache.iceberg.Transaction; import org.apache.iceberg.UpdateSchema; import org.apache.iceberg.catalog.Catalog; @@ -1176,4 +1179,35 @@ public void testDatabaseLocationWithSlashInWarehouseDir() { Assert.assertEquals("s3://bucket/database.db", database.getLocationUri()); } + + @Test + public void testRegisterTable() { + TableIdentifier identifier = TableIdentifier.of(DB_NAME, "t1"); + catalog.createTable(identifier, getTestSchema()); + Table registeringTable = catalog.loadTable(identifier); + catalog.dropTable(identifier, false); + TableOperations ops = ((HasTableOperations) registeringTable).operations(); + String metadataLocation = ((HiveTableOperations) ops).currentMetadataLocation(); + Table registeredTable = catalog.registerTable(identifier, metadataLocation); + assertThat(registeredTable).isNotNull(); + TestHelpers.assertSerializedAndLoadedMetadata(registeringTable, registeredTable); + String expectedMetadataLocation = + ((HasTableOperations) registeredTable).operations().current().metadataFileLocation(); + assertThat(metadataLocation).isEqualTo(expectedMetadataLocation); + assertThat(catalog.loadTable(identifier)).isNotNull(); + assertThat(catalog.dropTable(identifier)).isTrue(); + } + + @Test + public void testRegisterExistingTable() { + TableIdentifier identifier = TableIdentifier.of(DB_NAME, "t1"); + catalog.createTable(identifier, getTestSchema()); + Table registeringTable = catalog.loadTable(identifier); + TableOperations ops = ((HasTableOperations) registeringTable).operations(); + String metadataLocation = ((HiveTableOperations) ops).currentMetadataLocation(); + assertThatThrownBy(() -> catalog.registerTable(identifier, metadataLocation)) + .isInstanceOf(AlreadyExistsException.class) + .hasMessage("Table already exists: hivedb.t1"); + assertThat(catalog.dropTable(identifier, true)).isTrue(); + } } diff --git a/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java b/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java index 799960c2b6f7..4cc3547b6657 100644 --- a/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java +++ b/nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java @@ -143,10 +143,8 @@ protected void doRefresh() { @Override protected void doCommit(TableMetadata base, TableMetadata metadata) { - String newMetadataLocation = - (base == null) && (metadata.metadataFileLocation() != null) - ? metadata.metadataFileLocation() - : writeNewMetadata(metadata, currentVersion() + 1); + boolean newTable = base == null; + String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata); String refName = client.refName(); boolean delete = true;