[GitHub] [iceberg] kbendick commented on a change in pull request #4018: [HiveCatalog] Make Hive's external table behavior match with Iceberg table's gc.enabled behavior.

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

kbendick commented on a change in pull request #4018:
URL: https://github.com/apache/iceberg/pull/4018#discussion_r796463941



##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestCreateTable.java
##########
@@ -279,4 +284,88 @@ public void testDowngradeTableToFormatV1ThroughTablePropertyFails() {
         "Cannot downgrade v2 table to v1",
         () -> sql("ALTER TABLE %s SET TBLPROPERTIES ('format-version'='1')", tableName));
   }
+
+  @Test
+  public void testCreateExternalTableDisablesGC() throws IOException {
+    Assume.assumeTrue("Only test hive catalog", catalogName.equals(SparkCatalogConfig.HIVE.catalogName()));
+    Assert.assertFalse("Table should not already exist", validationCatalog.tableExists(tableIdent));
+
+    sql("CREATE EXTERNAL TABLE %s (id BIGINT NOT NULL, data STRING) USING iceberg", tableName);
+
+    Table table = validationCatalog.loadTable(tableIdent);
+    Assert.assertNotNull("Should load the new table", table);
+
+    File dataLocation = new File(table.location().replace("file:", "") + "/data");
+    Assert.assertFalse("No data should be there.", dataLocation.exists());
+
+    try {
+      sql("INSERT INTO %s VALUES (123456789, \"test string\")", tableName);
+      List<Object[]> expected = sql(String.format("SELECT * FROM %s", tableName));
+      Assert.assertEquals(1, expected.size());
+      Assert.assertEquals("Data should be there.", 2,
+          Objects.requireNonNull(dataLocation.listFiles()).length);
+
+      sql("DROP TABLE %s", tableName);
+      Assert.assertEquals("Data should not have been deleted.", 2,
+          Objects.requireNonNull(dataLocation.listFiles()).length);
+    } finally {
+      FileUtils.deleteDirectory(dataLocation.getParentFile());
+    }
+  }
+
+  @Test
+  public void testCreateExternalTableThrowsWhenGcSet() {
+    Assume.assumeTrue("Only test hive catalog", catalogName.equals(SparkCatalogConfig.HIVE.catalogName()));
+    Assert.assertFalse("Table should not already exist", validationCatalog.tableExists(tableIdent));
+
+    try {
+      sql("CREATE EXTERNAL TABLE %s (id BIGINT NOT NULL, data STRING) USING iceberg tblproperties(\"gc" +
+          ".enabled\"=\"true\")", tableName);
+      Assert.fail("Runtime exception should have been thrown.");
+    } catch (RuntimeException e) {
+      // expected, do nothing
+    }
+  }
+
+  @Test
+  public void testCreateManagedTableThrowsWhenGcNotSet() {
+    Assume.assumeTrue("Only test hive catalog", catalogName.equals(SparkCatalogConfig.HIVE.catalogName()));
+    Assert.assertFalse("Table should not already exist", validationCatalog.tableExists(tableIdent));
+
+    try {
+      sql("CREATE TABLE %s (id BIGINT NOT NULL, data STRING) USING iceberg tblproperties(\"gc" +
+          ".enabled\"=\"false\")", tableName);
+      Assert.fail("Runtime exception should have been thrown.");
+    } catch (RuntimeException e) {
+      // expected, do nothing
+    }
+  }
+
+  @Test
+  public void testCreateManagedTableEnablesGC() throws IOException {
+    Assume.assumeTrue("Only test hive catalog", catalogName.equals(SparkCatalogConfig.HIVE.catalogName()));
+    Assert.assertFalse("Table should not already exist", validationCatalog.tableExists(tableIdent));
+
+    sql("CREATE TABLE %s (id BIGINT NOT NULL, data STRING) USING iceberg", tableName);
+
+    Table table = validationCatalog.loadTable(tableIdent);
+    Assert.assertNotNull("Should load the new table", table);
+
+    File dataLocation = new File(table.location().replace("file:", "") + "/data");
+    Assert.assertFalse("No data should be there.", dataLocation.exists());
+
+    try {
+      sql("INSERT INTO %s VALUES (123456789, \"test string\")", tableName);
+      List<Object[]> expected = sql(String.format("SELECT * FROM %s", tableName));
+      Assert.assertEquals(1, expected.size());
+      Assert.assertEquals("Data should be there.", 2,
+          Objects.requireNonNull(dataLocation.listFiles()).length);
+
+      sql("DROP TABLE %s", tableName);
+      Assert.assertEquals("Data should not have been deleted.", 0,
+          Objects.requireNonNull(dataLocation.listFiles()).length);

Review comment:
       Can you please add a test or into this test a section that than then rebuilds the droppd table on top of the existing files?




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