[GitHub] [iceberg] theoxu31 commented on a diff in pull request #6742: AWS: support force register table in GlueCatalog

["theoxu31 (via GitHub)" <gi...@apache.org>]

theoxu31 commented on code in PR #6742:
URL: https://github.com/apache/iceberg/pull/6742#discussion_r1183164865


##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -563,25 +555,88 @@ public void testRegisterTable() {
     Table registeredTable = glueCatalog.registerTable(identifier, metadataLocation);
     Assertions.assertThat(registeredTable).isNotNull();
     String expectedMetadataLocation =
-        ((BaseTable) table).operations().current().metadataFileLocation();
+        ((BaseTable) registeredTable).operations().current().metadataFileLocation();
     Assertions.assertThat(metadataLocation).isEqualTo(expectedMetadataLocation);
+
+    GetTableResponse response =
+        glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
+    String actualMetadataLocationGlue =
+        response.table().parameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+
+    Assert.assertEquals(
+        "Glue Catalog Register Table should not submit a new commit",
+        expectedMetadataLocation,
+        actualMetadataLocationGlue);
+
     Assertions.assertThat(glueCatalog.loadTable(identifier)).isNotNull();
     Assertions.assertThat(glueCatalog.dropTable(identifier, true)).isTrue();
     Assertions.assertThat(glueCatalog.dropNamespace(Namespace.of(namespace))).isTrue();
   }
 
+  @Test
+  public void testRegisterTableNamespaceNotFound() {
+    String namespace = createNamespace();
+    String tableName = getRandomName();
+    createTable(namespace, tableName);
+    Table table =
+        glueCatalogWithForceRegisterTable.loadTable(TableIdentifier.of(namespace, tableName));
+    String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+    AssertHelpers.assertThrows(
+        "Should fail to register to an unknown namespace",
+        NoSuchNamespaceException.class,
+        "not found in Glue",
+        () ->
+            glueCatalogWithForceRegisterTable.registerTable(
+                TableIdentifier.of(getRandomName(), getRandomName()), metadataLocation));
+  }
+
   @Test
   public void testRegisterTableAlreadyExists() {
     String namespace = createNamespace();
     String tableName = getRandomName();
     createTable(namespace, tableName);
     TableIdentifier identifier = TableIdentifier.of(namespace, tableName);
-    Table table = glueCatalog.loadTable(identifier);
+    Table table = glueCatalogWithForceRegisterTable.loadTable(identifier);
     String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
-    Assertions.assertThatThrownBy(() -> glueCatalog.registerTable(identifier, metadataLocation))
-        .isInstanceOf(AlreadyExistsException.class);
-    Assertions.assertThat(glueCatalog.dropTable(identifier, true)).isTrue();
-    Assertions.assertThat(glueCatalog.dropNamespace(Namespace.of(namespace))).isTrue();
+    Assertions.assertThat(glueCatalogWithForceRegisterTable.dropTable(identifier, false)).isTrue();
+    Table registeredTable =
+        glueCatalogWithForceRegisterTable.registerTable(identifier, metadataLocation);
+    Assertions.assertThat(registeredTable).isNotNull();
+
+    GetTableResponse response =
+        glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
+    Assert.assertEquals(
+        "external table type is set after register",
+        "EXTERNAL_TABLE",
+        response.table().tableType());
+    String actualMetadataLocation =
+        response.table().parameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+    Assert.assertEquals(
+        "metadata location should be updated with registerTable call",
+        metadataLocation,
+        actualMetadataLocation);
+
+    // commit new transaction, should create a new metadata file
+    DataFile dataFile =
+        DataFiles.builder(partitionSpec)
+            .withPath("/path/to/data-a.parquet")
+            .withFileSizeInBytes(10)
+            .withRecordCount(1)
+            .build();
+    table.newAppend().appendFile(dataFile).commit();
+
+    metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+    // update metadata location
+    glueCatalogWithForceRegisterTable.registerTable(identifier, metadataLocation);
+    response =
+        glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
+    String updatedMetadataLocation =
+        response.table().parameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+    Assert.assertEquals(
+        "metadata location should be updated with registerTable call",
+        metadataLocation,
+        updatedMetadataLocation);
+    Assert.assertEquals("Table Version should be updated", "2", response.table().versionId());

Review Comment:
   Yes that's a good call out, I have separated into two tests in the new revision commits, thanks!



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org