hive git commit: HIVE-17942 : HiveAlterHandler not using conf from HMS Handler (Janaki Lahorani, reviewed by Alexander Kolbasov, Vihang Karajgaonkar)

[vi...@apache.org]

Repository: hive
Updated Branches:
  refs/heads/branch-2 fc4fe31a8 -> 4c451694b


HIVE-17942 : HiveAlterHandler not using conf from HMS Handler (Janaki Lahorani, reviewed by Alexander Kolbasov, Vihang Karajgaonkar)


Project: http://git-wip-us.apache.org/repos/asf/hive/repo
Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/4c451694
Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/4c451694
Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/4c451694

Branch: refs/heads/branch-2
Commit: 4c451694bcdad6d026fb2b5684c054eaeae9e39b
Parents: fc4fe31
Author: Vihang Karajgaonkar <vi...@cloudera.com>
Authored: Thu Nov 16 19:47:30 2017 -0800
Committer: Vihang Karajgaonkar <vi...@cloudera.com>
Committed: Thu Nov 16 19:47:45 2017 -0800

----------------------------------------------------------------------
 .../TestHiveMetaStoreAlterColumnPar.java        | 115 +++++++++++++++++++
 .../hadoop/hive/metastore/HiveAlterHandler.java |  18 +--
 2 files changed, 126 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hive/blob/4c451694/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
----------------------------------------------------------------------
diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
new file mode 100644
index 0000000..569e932
--- /dev/null
+++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreAlterColumnPar.java
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hive.jdbc.miniHS2.MiniHS2;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.Statement;
+import java.util.HashMap;
+
+/**
+ * Test that set/unset of metaconf:hive.metastore.disallow.incompatible.col.type.changes is local
+ * to the session
+ *
+ * Two connections are created.  In one connection
+ * metaconf:hive.metastore.disallow.incompatible.col.type.changes is set to false.  Then the
+ * columm type can changed from int to smallint.  In the other connection
+ * metaconf:hive.metastore.disallow.incompatible.col.type.changes is left with the default value
+ * true.  In that connection changing column type from int to smallint will throw an error.
+ *
+ */
+public class TestHiveMetaStoreAlterColumnPar {
+
+  public static MiniHS2 miniHS2 = null;
+
+  @BeforeClass
+  public static void startServices() throws Exception {
+    HiveConf hiveConf = new HiveConf();
+    hiveConf.setIntVar(ConfVars.HIVE_SERVER2_THRIFT_MIN_WORKER_THREADS, 2);
+    hiveConf.setIntVar(ConfVars.HIVE_SERVER2_THRIFT_MAX_WORKER_THREADS, 2);
+    hiveConf.setBoolVar(ConfVars.HIVE_SUPPORT_CONCURRENCY, false);
+
+    miniHS2 = new MiniHS2.Builder().withMiniMR().withRemoteMetastore().withConf(hiveConf).build();
+
+    miniHS2.start(new HashMap<String, String>());
+
+    Connection hs2Conn = DriverManager.getConnection(miniHS2.getJdbcURL(), "hive", "hive");
+    Statement stmt = hs2Conn.createStatement();
+
+    // create table
+    stmt.execute("drop table if exists t1");
+    stmt.execute("create table t1 (c1 int)");
+
+    stmt.close();
+    hs2Conn.close();
+  }
+
+  @AfterClass
+  public static void stopServices() throws Exception {
+    if (miniHS2 != null && miniHS2.isStarted()) {
+      miniHS2.stop();
+    }
+  }
+
+  @Test
+  public void testAlterColumn() throws Exception {
+    assertTrue("Test setup failed. MiniHS2 is not initialized",
+        miniHS2 != null && miniHS2.isStarted());
+
+    try (
+        Connection hs2Conn1 = DriverManager
+            .getConnection(TestHiveMetaStoreAlterColumnPar.miniHS2.getJdbcURL(), "hive", "hive");
+        Statement stmt1 = hs2Conn1.createStatement();
+        Connection hs2Conn2 = DriverManager
+            .getConnection(TestHiveMetaStoreAlterColumnPar.miniHS2.getJdbcURL(), "hive", "hive");
+        Statement stmt2 = hs2Conn2.createStatement();
+    ) {
+      // Set parameter to be false in connection 1.  int to smallint allowed
+      stmt1.execute("set metaconf:hive.metastore.disallow.incompatible.col.type.changes=false");
+      stmt1.execute("alter table t1 change column c1 c1 smallint");
+
+      // Change the type back to int so that the same alter can be attempted from connection 2.
+      stmt1.execute("alter table t1 change column c1 c1 int");
+
+      // parameter value not changed to false in connection 2.  int to smallint throws exception
+      try {
+        stmt2.execute("alter table t1 change column c1 c1 smallint");
+        assertTrue("Exception not thrown", true);
+      } catch (Exception e1) {
+        assertTrue("Unexpected exception: " + e1.getMessage(), e1.getMessage().contains(
+            "Unable to alter table. The following columns have types incompatible with the existing columns in their respective positions"));
+      }
+
+      // parameter value is still false in 1st connection.  The alter still goes through.
+      stmt1.execute("alter table t1 change column c1 c1 smallint");
+    } catch (Exception e2) {
+      assertTrue("Unexpected Exception: " + e2.getMessage(), true);
+    }
+  }
+}

http://git-wip-us.apache.org/repos/asf/hive/blob/4c451694/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
----------------------------------------------------------------------
diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
index 79013e9..3e7c59b 100644
--- a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
+++ b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
@@ -69,6 +69,10 @@ public class HiveAlterHandler implements AlterHandler {
   private static final Logger LOG = LoggerFactory.getLogger(HiveAlterHandler.class
       .getName());
 
+  // hiveConf, getConf and setConf are in this class because AlterHandler extends Configurable.
+  // Always use the configuration from HMS Handler.  Making AlterHandler not extend Configurable
+  // is not in the scope of the fix for HIVE-17942.
+
   @Override
   public Configuration getConf() {
     return hiveConf;
@@ -105,7 +109,7 @@ public class HiveAlterHandler implements AlterHandler {
     String newTblName = newt.getTableName().toLowerCase();
     String newDbName = newt.getDbName().toLowerCase();
 
-    if (!MetaStoreUtils.validateName(newTblName, hiveConf)) {
+    if (!MetaStoreUtils.validateName(newTblName, handler.getConf())) {
       throw new InvalidOperationException(newTblName + " is not a valid object name");
     }
     String validate = MetaStoreUtils.validateTblColumns(newt.getSd().getCols());
@@ -155,7 +159,7 @@ public class HiveAlterHandler implements AlterHandler {
       // can be altered to change the column types.  The column type compatibility checks should
       // be done only for non-views.
       if (HiveConf
-          .getBoolVar(hiveConf, HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES,
+          .getBoolVar(handler.getConf(), HiveConf.ConfVars.METASTORE_DISALLOW_INCOMPATIBLE_COL_TYPE_CHANGES,
               false) && !oldt.getTableType().equals(TableType.VIRTUAL_VIEW.toString())) {
         // Throws InvalidOperationException if the new column types are not
         // compatible with the current column types.
@@ -276,7 +280,7 @@ public class HiveAlterHandler implements AlterHandler {
         }
       } else {
         // operations other than table rename
-        if (MetaStoreUtils.requireCalStats(hiveConf, null, null, newt, environmentContext) &&
+        if (MetaStoreUtils.requireCalStats(handler.getConf(), null, null, newt, environmentContext) &&
             !isPartitionedTable) {
           Database db = msdb.getDatabase(newDbName);
           // Update table stats. For partitioned table, we update stats in alterPartition()
@@ -404,7 +408,7 @@ public class HiveAlterHandler implements AlterHandler {
       try {
         msdb.openTransaction();
         oldPart = msdb.getPartition(dbname, name, new_part.getValues());
-        if (MetaStoreUtils.requireCalStats(hiveConf, oldPart, new_part, tbl, environmentContext)) {
+        if (MetaStoreUtils.requireCalStats(handler.getConf(), oldPart, new_part, tbl, environmentContext)) {
           // if stats are same, no need to update
           if (MetaStoreUtils.isFastStatsSame(oldPart, new_part)) {
             MetaStoreUtils.updateBasicState(environmentContext, new_part.getParameters());
@@ -518,7 +522,7 @@ public class HiveAlterHandler implements AlterHandler {
               }
 
               //rename the data directory
-              wh.renameDir(srcPath, destPath, FileUtils.shouldInheritPerms(hiveConf, destFs));
+              wh.renameDir(srcPath, destPath, FileUtils.shouldInheritPerms(handler.getConf(), destFs));
               LOG.info("Partition directory rename from " + srcPath + " to " + destPath + " done.");
               dataWasMoved = true;
             }
@@ -537,7 +541,7 @@ public class HiveAlterHandler implements AlterHandler {
         new_part.getSd().setLocation(oldPart.getSd().getLocation());
       }
 
-      if (MetaStoreUtils.requireCalStats(hiveConf, oldPart, new_part, tbl, environmentContext)) {
+      if (MetaStoreUtils.requireCalStats(handler.getConf(), oldPart, new_part, tbl, environmentContext)) {
         MetaStoreUtils.updatePartitionStatsFast(new_part, wh, false, true, environmentContext);
       }
 
@@ -629,7 +633,7 @@ public class HiveAlterHandler implements AlterHandler {
         oldParts.add(oldTmpPart);
         partValsList.add(tmpPart.getValues());
 
-        if (MetaStoreUtils.requireCalStats(hiveConf, oldTmpPart, tmpPart, tbl, environmentContext)) {
+        if (MetaStoreUtils.requireCalStats(handler.getConf(), oldTmpPart, tmpPart, tbl, environmentContext)) {
           // Check if stats are same, no need to update
           if (MetaStoreUtils.isFastStatsSame(oldTmpPart, tmpPart)) {
             MetaStoreUtils.updateBasicState(environmentContext, tmpPart.getParameters());