[GitHub] [iceberg] nastra commented on a diff in pull request #5510: Support delete corrupted Iceberg table

[GitBox <gi...@apache.org>]

nastra commented on code in PR #5510:
URL: https://github.com/apache/iceberg/pull/5510#discussion_r949979992


##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -169,9 +170,14 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
 
     TableOperations ops = newTableOps(identifier);
     TableMetadata lastMetadata;
-    if (purge && ops.current() != null) {
-      lastMetadata = ops.current();
-    } else {
+    try {
+      if (purge && ops.current() != null) {
+        lastMetadata = ops.current();
+      } else {
+        lastMetadata = null;
+      }
+    } catch (NotFoundException e) {
+      LOG.warn("The metadata file doesn't exist any more. Continue to drop the table.", e);

Review Comment:
   ```suggestion
         LOG.warn("The metadata file doesn't exist anymore. Continuing to drop the table.", e);
   ```



##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -458,6 +460,36 @@ public void testDropNamespace() throws TException {
         });
   }
 
+  @Test
+  public void testDropTable() throws TException {
+    Namespace namespace = Namespace.of("dbname_drop");
+    TableIdentifier identifier = TableIdentifier.of(namespace, "table");
+    Schema tableSchema =
+        new Schema(Types.StructType.of(required(1, "id", Types.LongType.get())).fields());
+    catalog.createNamespace(namespace, meta);
+    catalog.createTable(identifier, tableSchema);
+    TableOperations ops = catalog.newTableOps(identifier);
+
+    Assert.assertTrue(catalog.dropTable(identifier, true));
+    AssertHelpers.assertThrows(
+        "Should fail to load table after dropping it",
+        NoSuchTableException.class,
+        () -> catalog.loadTable(identifier));
+
+    // delete corrupted table

Review Comment:
   maybe just `// delete metadata file`



##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -458,6 +460,36 @@ public void testDropNamespace() throws TException {
         });
   }
 
+  @Test
+  public void testDropTable() throws TException {
+    Namespace namespace = Namespace.of("dbname_drop");
+    TableIdentifier identifier = TableIdentifier.of(namespace, "table");
+    Schema tableSchema =
+        new Schema(Types.StructType.of(required(1, "id", Types.LongType.get())).fields());
+    catalog.createNamespace(namespace, meta);
+    catalog.createTable(identifier, tableSchema);
+    TableOperations ops = catalog.newTableOps(identifier);
+
+    Assert.assertTrue(catalog.dropTable(identifier, true));

Review Comment:
   I agree, let's remove L473 - 478 and just focus on the actual case where we don't have a metadata file for the given table



##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -458,6 +460,36 @@ public void testDropNamespace() throws TException {
         });
   }
 
+  @Test
+  public void testDropTable() throws TException {

Review Comment:
   ```suggestion
     public void testDropTableWithoutMetadataFile() throws TException {
   ```



##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -458,6 +460,36 @@ public void testDropNamespace() throws TException {
         });
   }
 
+  @Test
+  public void testDropTable() throws TException {
+    Namespace namespace = Namespace.of("dbname_drop");
+    TableIdentifier identifier = TableIdentifier.of(namespace, "table");
+    Schema tableSchema =
+        new Schema(Types.StructType.of(required(1, "id", Types.LongType.get())).fields());
+    catalog.createNamespace(namespace, meta);
+    catalog.createTable(identifier, tableSchema);
+    TableOperations ops = catalog.newTableOps(identifier);
+
+    Assert.assertTrue(catalog.dropTable(identifier, true));
+    AssertHelpers.assertThrows(
+        "Should fail to load table after dropping it",
+        NoSuchTableException.class,
+        () -> catalog.loadTable(identifier));
+
+    // delete corrupted table
+    catalog.createTable(identifier, tableSchema);
+    String metadataFileLocation = catalog.newTableOps(identifier).current().metadataFileLocation();
+    ops.io().deleteFile(metadataFileLocation);
+    Assert.assertTrue(catalog.dropTable(identifier, true));
+    AssertHelpers.assertThrows(
+        "Should fail to load table after dropping it",
+        NoSuchTableException.class,

Review Comment:
   nit: let's use
   ```
   Assertions.assertThatThrownBy(() -> catalog.loadTable(identifier)).isInstanceOf(NoSuchTableException.class).hasMessage("xx")
   ```



-- 
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