From b5150f3b5d7eee6d8461ddeabe274d1755df1100 Mon Sep 17 00:00:00 2001
From: Marton Bod <mbod@cloudera.com>
Date: Thu, 18 Feb 2021 16:18:38 +0100
Subject: [PATCH 1/3] Hive: removed Iceberg props should be removed from HMS
 table props too

---
 .../iceberg/hive/HiveTableOperations.java       | 17 +++++++++++++++--
 .../TestHiveIcebergStorageHandlerNoScan.java    | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

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 f03fa0b1014e..d25c5faae87c 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
@@ -23,11 +23,13 @@
 import java.net.UnknownHostException;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hive.common.StatsSetupConst;
 import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
@@ -186,7 +188,15 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
             baseMetadataLocation, metadataLocation, database, tableName);
       }
 
-      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), hiveEngineEnabled);
+      // get Iceberg props that have been removed
+      List<String> removedProps = Collections.emptyList();
+      if (base != null) {
+        removedProps = base.properties().keySet().stream()
+            .filter(key -> !metadata.properties().containsKey(key))
+            .collect(Collectors.toList());
+      }
+
+      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled);
 
       persistTable(tbl, updateHiveTable);
       threw = false;
@@ -258,7 +268,7 @@ private Table newHmsTable() {
   }
 
   private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<String, String> icebergTableProps,
-      boolean hiveEngineEnabled) {
+      List<String> obsoleteProps, boolean hiveEngineEnabled) {
     Map<String, String> parameters = tbl.getParameters();
 
     if (parameters == null) {
@@ -268,6 +278,9 @@ private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<St
     // push all Iceberg table properties into HMS
     icebergTableProps.forEach(parameters::put);
 
+    // remove any props from HMS that are no longer present in Iceberg table props
+    obsoleteProps.forEach(parameters::remove);
+
     parameters.put(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH));
     parameters.put(METADATA_LOCATION_PROP, newMetadataLocation);
 
diff --git a/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java b/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java
index a57bf18963f8..ac6a150b3fd9 100644
--- a/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java
+++ b/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java
@@ -569,6 +569,21 @@ public void testIcebergAndHmsTableProperties() throws TException, InterruptedExc
     } else {
       Assert.assertEquals(7, hmsParams.size());
     }
+
+    // Remove some Iceberg props and see if they're removed from HMS table props as well
+    if (Catalogs.hiveCatalog(shell.getHiveConf())) {
+      icebergTable.updateProperties()
+          .remove("custom_property")
+          .remove("new_prop_1")
+          .commit();
+      hmsParams = shell.metastore().getTable("default", "customers").getParameters()
+          .entrySet().stream()
+          .filter(e -> !IGNORED_PARAMS.contains(e.getKey()))
+          .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+      Assert.assertFalse(hmsParams.containsKey("custom_property"));
+      Assert.assertFalse(hmsParams.containsKey("new_prop_1"));
+      Assert.assertTrue(hmsParams.containsKey("new_prop_2"));
+    }
   }
 
   @Test

From b69c42244149ce6965c7a38d9ff5bb2838d1dc9a Mon Sep 17 00:00:00 2001
From: Marton Bod <mbod@cloudera.com>
Date: Thu, 18 Feb 2021 18:35:27 +0100
Subject: [PATCH 2/3] Change from list to set

---
 .../java/org/apache/iceberg/hive/HiveTableOperations.java | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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 d25c5faae87c..2d2c6966f0e5 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
@@ -23,11 +23,11 @@
 import java.net.UnknownHostException;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import org.apache.hadoop.conf.Configuration;
@@ -189,11 +189,11 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
       }
 
       // get Iceberg props that have been removed
-      List<String> removedProps = Collections.emptyList();
+      Set<String> removedProps = Collections.emptySet();
       if (base != null) {
         removedProps = base.properties().keySet().stream()
             .filter(key -> !metadata.properties().containsKey(key))
-            .collect(Collectors.toList());
+            .collect(Collectors.toSet());
       }
 
       setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled);
@@ -268,7 +268,7 @@ private Table newHmsTable() {
   }
 
   private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<String, String> icebergTableProps,
-      List<String> obsoleteProps, boolean hiveEngineEnabled) {
+      Set<String> obsoleteProps, boolean hiveEngineEnabled) {
     Map<String, String> parameters = tbl.getParameters();
 
     if (parameters == null) {

From e91c8b17b4f283c11067624610e1eefaffb4d42c Mon Sep 17 00:00:00 2001
From: Marton Bod <mbod@cloudera.com>
Date: Fri, 19 Feb 2021 09:32:11 +0100
Subject: [PATCH 3/3] adjust method header whitespace

---
 .../main/java/org/apache/iceberg/hive/HiveTableOperations.java  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 2d2c6966f0e5..2c8c9aedf791 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,7 +268,7 @@ private Table newHmsTable() {
   }
 
   private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<String, String> icebergTableProps,
-      Set<String> obsoleteProps, boolean hiveEngineEnabled) {
+                                     Set<String> obsoleteProps, boolean hiveEngineEnabled) {
     Map<String, String> parameters = tbl.getParameters();
 
     if (parameters == null) {