[iceberg] branch master updated: Hive: removed Iceberg props should be removed from HMS table props too (#2252)

[pv...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

pvary pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/master by this push:
     new efc4618  Hive: removed Iceberg props should be removed from HMS table props too (#2252)
efc4618 is described below

commit efc461862fc93637fe4c0127d69bfb777c87766e
Author: Marton Bod <ma...@gmail.com>
AuthorDate: Fri Feb 19 11:29:15 2021 +0100

    Hive: removed Iceberg props should be removed from HMS table props too (#2252)
---
 .../org/apache/iceberg/hive/HiveTableOperations.java    | 17 +++++++++++++++--
 .../mr/hive/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 f03fa0b..2c8c9ae 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
@@ -27,7 +27,9 @@ 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;
 import org.apache.hadoop.hive.common.StatsSetupConst;
 import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
@@ -186,7 +188,15 @@ public class HiveTableOperations extends BaseMetastoreTableOperations {
             baseMetadataLocation, metadataLocation, database, tableName);
       }
 
-      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), hiveEngineEnabled);
+      // get Iceberg props that have been removed
+      Set<String> removedProps = Collections.emptySet();
+      if (base != null) {
+        removedProps = base.properties().keySet().stream()
+            .filter(key -> !metadata.properties().containsKey(key))
+            .collect(Collectors.toSet());
+      }
+
+      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled);
 
       persistTable(tbl, updateHiveTable);
       threw = false;
@@ -258,7 +268,7 @@ public class HiveTableOperations extends BaseMetastoreTableOperations {
   }
 
   private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<String, String> icebergTableProps,
-      boolean hiveEngineEnabled) {
+                                     Set<String> obsoleteProps, boolean hiveEngineEnabled) {
     Map<String, String> parameters = tbl.getParameters();
 
     if (parameters == null) {
@@ -268,6 +278,9 @@ public class HiveTableOperations extends BaseMetastoreTableOperations {
     // 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 a57bf18..ac6a150 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 class TestHiveIcebergStorageHandlerNoScan {
     } 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