[impala] 01/03: IMPALA-9990: Support SET OWNER for Kudu tables

[as...@apache.org]

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

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

commit eda06f41ce004cfc7a35d7d26d93e76d54c7cdb0
Author: Fang-Yu Rao <fa...@cloudera.com>
AuthorDate: Sat Aug 1 11:24:08 2020 -0700

    IMPALA-9990: Support SET OWNER for Kudu tables
    
    KUDU-3090 adds the support for table ownership and exposes the API's of
    setting owner on creating and altering tables, which allows Impala to
    also pass to Kudu the new owner of the Kudu table for the ALTER TABLE
    SET OWNER statement.
    
    Specifically, based on the API of AlterTableOptions#setOwner(), this
    patch stores the ownership information of the Kudu table in the
    corresponding instance of AlterTableOptions, which will then be passed
    to Kudu via a KuduClient.
    
    Testing:
    - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement
      could be correctly analyzed.
    - Added an E2E test in kudu_alter.test to verify the statement could be
      correctly executed when the integration between Kudu and HMS is not
      enabled.
    - Added an E2E test in kudu_hms_alter.test and verified that the
      statement could be correctly executed when the integration between
      Kudu and HMS is enabled after manually re-enabling
      TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was
      not possible before IMPALA-10092 was resolved due to a bug in the
      class of CustomClusterTestSuite. In addition, we may need to delete
      the Kudu table 'simple' via a Kudu-Python client if the E2E test
      complains that the Kudu table already exists, which may be related to
      IMPALA-8751.
    - Manually verified that the views of Kudu server and HMS are consistent
      for a synchronized Kudu table after the ALTER TABLE SET OWNER
      statement even though the Kudu table was once an external and
      non-synchronized table, meaning that the owner from Kudu's perspective
      could be different than that from HMS' perspective. Such a discrepancy
      could be created if we execute the ALTER TABLE SET OWNER statement for
      an external Kudu table with the property of 'external.table.purge'
      being false. The test is performed manually because currently the
      Kudu-Python client adopted in Impala's E2E tests is not up to date so
      that the field of 'owner' cannot be accessed in the E2E tests. On the
      other hand, to verify the owner of a Kudu table from Kudu's
      perspective, we used the latest Kudu-Python client as provided at
      github.com/apache/kudu/tree/master/examples/python/basic-python-example.
    - Verified that the patch could pass the exhaustive tests in the DEBUG
      mode.
    
    Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c
    Reviewed-on: http://gerrit.cloudera.org:8080/16273
    Reviewed-by: Vihang Karajgaonkar <vi...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../org/apache/impala/service/CatalogOpExecutor.java     | 16 +++++++++++++++-
 .../org/apache/impala/service/KuduCatalogOpExecutor.java | 15 +++++++++++++++
 .../org/apache/impala/analysis/AnalyzeKuduDDLTest.java   |  3 +++
 .../functional-query/queries/QueryTest/kudu_alter.test   | 16 ++++++++++++++++
 .../queries/QueryTest/kudu_hms_alter.test                | 16 ++++++++++++++++
 5 files changed, 65 insertions(+), 1 deletion(-)

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 05ff417..443b8a5 100644
--- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
@@ -3778,7 +3778,21 @@ public class CatalogOpExecutor {
     PrincipalType oldOwnerType = msTbl.getOwnerType();
     msTbl.setOwner(params.owner_name);
     msTbl.setOwnerType(PrincipalType.valueOf(params.owner_type.name()));
-    applyAlterTable(msTbl);
+
+    // A KuduTable is synchronized if it is a managed KuduTable, or an external table
+    // with the property of 'external.table.purge' being true.
+    boolean isSynchronizedKuduTable = (tbl instanceof KuduTable) &&
+        KuduTable.isSynchronizedTable(msTbl);
+    boolean altersHMSTable = true;
+    if (isSynchronizedKuduTable) {
+      boolean isKuduHmsIntegrationEnabled = isKuduHmsIntegrationEnabled(msTbl);
+      // We need to update HMS when the integration between Kudu and HMS is not enabled.
+      altersHMSTable = !isKuduHmsIntegrationEnabled;
+      KuduCatalogOpExecutor.alterSetOwner((KuduTable) tbl, params.owner_name);
+    }
+
+    if (altersHMSTable) applyAlterTable(msTbl);
+
     if (authzConfig_.isEnabled()) {
       authzManager_.updateTableOwnerPrivilege(params.server_name, msTbl.getDbName(),
           msTbl.getTableName(), oldOwner, oldOwnerType, msTbl.getOwner(),
diff --git a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
index 15eb986..1c49f72 100644
--- a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
@@ -542,6 +542,21 @@ public class KuduCatalogOpExecutor {
     alterKuduTable(tbl, alterTableOptions, errMsg);
   }
 
+  public static void alterSetOwner(KuduTable tbl, String newOwner)
+      throws ImpalaRuntimeException {
+    // We still need to call alterKuduTable() even if tbl.getOwnerUser() equals
+    // 'newOwner'. It is possible that the owner of 'tbl' from Kudu's perspective is
+    // different than that from HMS' perspective, i.e., tbl.getOwnerUser(). Such a
+    // discrepancy is possible if 'tbl' is changed to a synchronized Kudu table from an
+    // external, non-synchronized table before this method is called.
+
+    AlterTableOptions alterTableOptions = new AlterTableOptions();
+    alterTableOptions.setOwner(newOwner);
+    String errMsg = String.format(
+        "Error setting the owner of Kudu table %s to %s", tbl.getName(), newOwner);
+    alterKuduTable(tbl, alterTableOptions, errMsg);
+  }
+
   /**
    * Alters a Kudu table based on the specified AlterTableOptions params. Blocks until
    * the alter table operation is finished or until the operation timeout is reached.
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
index 40334b1..e81b5b0 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
@@ -722,5 +722,8 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase {
     AnalysisError("alter table functional_kudu.alltypes set tblproperties("
         + "'sort.order'='true')",
         "'sort.*' table properties are not supported for Kudu tables.");
+
+    // ALTER TABLE SET OWNER USER
+    AnalyzesOk("ALTER TABLE functional_kudu.testtbl SET OWNER USER new_owner");
   }
 }
diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
index cf84138..a13c728 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
@@ -48,6 +48,22 @@ alter table simple set tblproperties ('kudu.master_addresses' = 'invalid_host')
 ImpalaRuntimeException: Kudu table 'impala::$DATABASE.simple' does not exist on master 'invalid_host'
 ====
 ---- QUERY
+alter table simple set owner user new_owner
+---- RESULTS
+'Updated table/view.'
+---- TYPES
+STRING
+====
+---- QUERY
+# The change to the owner should be reflected whether or not the integration between Kudu
+# and HMS is enabled.
+describe formatted simple
+----RESULTS: VERIFY_IS_SUBSET
+'Owner:              ','new_owner           ','NULL'
+---- TYPES
+STRING, STRING, STRING
+====
+---- QUERY
 alter table simple rename to simple_new;
 ---- RESULTS
 'Renaming was successful.'
diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test
index 29efd26..244f69c 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test
@@ -48,6 +48,22 @@ alter table simple set tblproperties ('kudu.master_addresses' = 'invalid_host')
 ImpalaRuntimeException: Kudu table '$DATABASE.simple' does not exist on master 'invalid_host'
 ====
 ---- QUERY
+alter table simple set owner user new_owner
+---- RESULTS
+'Updated table/view.'
+---- TYPES
+STRING
+====
+---- QUERY
+# The change to the owner should be reflected whether or not the integration between Kudu
+# and HMS is enabled.
+describe formatted simple
+----RESULTS: VERIFY_IS_SUBSET
+'Owner:              ','new_owner           ','NULL'
+---- TYPES
+STRING, STRING, STRING
+====
+---- QUERY
 alter table simple rename to simple_new;
 ---- RESULTS
 'Renaming was successful.'