[impala] 05/05: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format

[st...@apache.org]

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

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

commit b40870d55b68b1307569380b135475824aeac2e4
Author: Zoltan Borok-Nagy <bo...@cloudera.com>
AuthorDate: Wed Mar 31 17:19:10 2021 +0200

    IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
    
    During table creation we pushed all the table properties to the
    Iceberg table, e.g. 'iceberg.file_format'. This works well with
    current Iceberg version, but causes bugs with newer versions.
    
    Newer versions of Iceberg refresh the HMS properties from the
    table properties for every table update. That means if we
    intially set a table property, we cannot modify it later because
    Iceberg will rewrite it with the old value on every table update.
    
    These table properties are not needed at the Iceberg table level,
    so we can just skip passing them during table creation.
    
    Testing:
     * ran existing tests
     * ran tests with newer version Iceberg
     * checked manually that we don't set these properties at the Iceberg
       table level
    
    Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
    Reviewed-on: http://gerrit.cloudera.org:8080/17248
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../apache/impala/service/CatalogOpExecutor.java   |  6 ++---
 .../impala/service/IcebergCatalogOpExecutor.java   | 30 +++++++++++++++++++++-
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
index 2a89b77..9c483ff 100644
--- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
@@ -298,9 +298,6 @@ public class CatalogOpExecutor {
   private final static String BLACKLISTED_TABLES_INCONSISTENT_ERR_STR =
       "--blacklisted_tables may be inconsistent between catalogd and coordinators";
 
-  // Table capabilities property name
-  private static final String CAPABILITIES_KEY = "OBJCAPABILITIES";
-
   // Table default capabilities
   private static final String ACIDINSERTONLY_CAPABILITIES =
       "HIVEMANAGEDINSERTREAD,HIVEMANAGEDINSERTWRITE";
@@ -314,6 +311,9 @@ public class CatalogOpExecutor {
   // PARTITION statement.
   public final static short MAX_PARTITION_UPDATES_PER_RPC = 500;
 
+  // Table capabilities property name
+  public static final String CAPABILITIES_KEY = "OBJCAPABILITIES";
+
   private final CatalogServiceCatalog catalog_;
   private final AuthorizationConfig authzConfig_;
   private final AuthorizationManager authzManager_;
diff --git a/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
index 370f84e..84f4b5c 100644
--- a/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
@@ -19,7 +19,9 @@ package org.apache.impala.service;
 
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.apache.iceberg.AppendFiles;
 import org.apache.iceberg.BaseTable;
@@ -73,7 +75,7 @@ public class IcebergCatalogOpExecutor {
         params.getPartition_spec());
     IcebergCatalog icebergCatalog = IcebergUtil.getIcebergCatalog(catalog, location);
     Table iceTable = icebergCatalog.createTable(identifier, schema, spec, location,
-        params.getTable_properties());
+        excludeHmsOnlyProps(params.getTable_properties()));
     LOG.info("Create iceberg table successful.");
     return iceTable;
   }
@@ -159,6 +161,32 @@ public class IcebergCatalogOpExecutor {
   }
 
   /**
+   * Returns a new Map without the properties that only need to be stored at the
+   * HMS level, not at the Iceberg table level.
+   */
+  private static Map<String, String> excludeHmsOnlyProps(Map<String, String> props) {
+    Map<String, String> ret = new HashMap<>();
+    for (Map.Entry<String, String> entry : props.entrySet()) {
+      if (isHmsOnlyProperty(entry.getKey())) continue;
+      ret.put(entry.getKey(), entry.getValue());
+    }
+    return ret;
+  }
+
+  /**
+   * Returns true if the table property should only be stored in HMS.
+   * If false, the property is stored in HMS as well as iceberg.
+   */
+  private static boolean isHmsOnlyProperty(String propKey) {
+    if (IcebergTable.ICEBERG_FILE_FORMAT.equals(propKey)) return true;
+    if (IcebergTable.ICEBERG_CATALOG.equals(propKey)) return true;
+    if (IcebergTable.ICEBERG_CATALOG_LOCATION.equals(propKey)) return true;
+    if (IcebergTable.ICEBERG_TABLE_IDENTIFIER.equals(propKey)) return true;
+    if (CatalogOpExecutor.CAPABILITIES_KEY.equals(propKey)) return true;
+    return false;
+  }
+
+  /**
    * Build iceberg schema by parameters.
    */
   private static Schema createIcebergSchema(TCreateTableParams params)