You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ar...@apache.org on 2018/07/09 19:50:08 UTC

[1/6] impala git commit: IMPALA-6988: Implement ALTER TABLE/VIEW SET OWNER

Repository: impala
Updated Branches:
  refs/heads/master 30d196fd5 -> 7a8f61c79


IMPALA-6988: Implement ALTER TABLE/VIEW SET OWNER

Alter the table/view owner to either user or role.

On table/view creation, the table/view owner will be set to
the current user, which can be viewed via DESCRIBE FORMATTED
command. Having an owner information allows implementing a
feature where an owner can be given certain privileges
automatically upon a table/view creation. See IMPALA-7075.
The ALTER TABLE/VIEW SET OWNER will be useful commands for
transferring ownership (a set of owner privileges) from the
current owner to another owner.

Syntax:
ALTER TABLE table SET OWNER USER user
ALTER TABLE table SET OWNER ROLE role

ALTER VIEW view SET OWNER USER user
ALTER VIEW view SET OWNER ROLE role

Testing:
- Added new FE tests
- Added new E2E tests

Change-Id: Ia1b75b1590b16eb0c2ba326d07ee3fd9897c27d1
Reviewed-on: http://gerrit.cloudera.org:8080/10822
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/54b3c607
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/54b3c607
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/54b3c607

Branch: refs/heads/master
Commit: 54b3c607f027c445fc8d52acc4d3b1073e84aa77
Parents: 30d196f
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Mon Jun 25 23:11:52 2018 -0500
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Sat Jul 7 01:32:03 2018 +0000

----------------------------------------------------------------------
 common/thrift/JniCatalog.thrift                 | 13 ++++
 fe/src/main/cup/sql-parser.cup                  | 36 +++++++++-
 .../analysis/AlterTableOrViewSetOwnerStmt.java  | 72 ++++++++++++++++++++
 .../impala/analysis/AlterTableSetOwnerStmt.java | 37 ++++++++++
 .../apache/impala/analysis/AlterTableStmt.java  |  2 +-
 .../impala/analysis/AlterViewSetOwnerStmt.java  | 37 ++++++++++
 .../impala/service/CatalogOpExecutor.java       | 16 ++++-
 .../apache/impala/analysis/AnalyzeDDLTest.java  | 38 +++++++++++
 .../impala/analysis/AuthorizationStmtTest.java  | 23 ++++++-
 .../org/apache/impala/analysis/ParserTest.java  | 26 +++++++
 tests/metadata/test_ddl.py                      | 22 ++++++
 tests/metadata/test_ddl_base.py                 | 19 ++++++
 12 files changed, 335 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/common/thrift/JniCatalog.thrift
----------------------------------------------------------------------
diff --git a/common/thrift/JniCatalog.thrift b/common/thrift/JniCatalog.thrift
index 1876138..4a8298b 100644
--- a/common/thrift/JniCatalog.thrift
+++ b/common/thrift/JniCatalog.thrift
@@ -104,6 +104,7 @@ enum TAlterTableType {
   SET_CACHED,
   RECOVER_PARTITIONS,
   SET_ROW_FORMAT,
+  SET_OWNER
 }
 
 // Parameters of CREATE DATABASE commands
@@ -317,6 +318,15 @@ struct TAlterTableSetLocationParams {
   2: optional list<CatalogObjects.TPartitionKeyValue> partition_spec
 }
 
+// Parameters for ALTER TABLE/VIEW SET OWNER commands.
+struct TAlterTableOrViewSetOwnerParams {
+  // The owner type.
+  1: required TOwnerType owner_type
+
+  // The owner name.
+  2: required string owner_name
+}
+
 // Parameters for updating the table and/or column statistics
 // of a table. Used by ALTER TABLE SET COLUMN STATS, and internally by
 // a COMPUTE STATS command.
@@ -397,6 +407,9 @@ struct TAlterTableParams {
 
   // Parameters for ALTER TABLE SET ROW FORMAT
   15: optional TAlterTableSetRowFormatParams set_row_format_params
+
+  // Parameters for ALTER TABLE/VIEW SET OWNER
+  16: optional TAlterTableOrViewSetOwnerParams set_owner_params
 }
 
 // Parameters of CREATE TABLE LIKE commands

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/cup/sql-parser.cup
----------------------------------------------------------------------
diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup
index bd00ad4..94b69e4 100644
--- a/fe/src/main/cup/sql-parser.cup
+++ b/fe/src/main/cup/sql-parser.cup
@@ -1058,6 +1058,9 @@ alter_db_stmt ::=
   :}
   ;
 
+// In some places, the opt_partition_set is used to avoid conflicts even though
+// a partition clause does not make sense for this stmt. If a partition
+// is given, manually throw a parse error.
 alter_tbl_stmt ::=
   KW_ALTER KW_TABLE table_name:table replace_existing_cols_val:replace KW_COLUMNS
   LPAREN column_def_list:col_defs RPAREN
@@ -1107,9 +1110,7 @@ alter_tbl_stmt ::=
   | KW_ALTER KW_TABLE table_name:table opt_partition_set:partition KW_SET
     KW_COLUMN KW_STATS ident_or_default:col LPAREN properties_map:map RPAREN
   {:
-    // The opt_partition_set is used to avoid conflicts even though
-    // a partition clause does not make sense for this stmt. If a partition
-    // is given, manually throw a parse error.
+    // See above for special partition clause handling.
     if (partition != null) parser.parseError("set", SqlParserSymbols.KW_SET);
     RESULT = new AlterTableSetColumnStats(table, col, map);
   :}
@@ -1134,6 +1135,23 @@ alter_tbl_stmt ::=
   | KW_ALTER KW_TABLE table_name:table KW_ALTER opt_kw_column ident_or_default:col_name
     KW_DROP KW_DEFAULT
   {: RESULT = AlterTableAlterColStmt.createDropDefaultStmt(table, col_name); :}
+  | KW_ALTER KW_TABLE table_name:table opt_partition_set:partitions KW_SET IDENT:owner_id
+    IDENT:user_id ident_or_default:user
+  {:
+    // See above for special partition clause handling.
+    if (partitions != null) parser.parseError("set", SqlParserSymbols.KW_SET);
+    parser.checkIdentKeyword("OWNER", owner_id);
+    parser.checkIdentKeyword("USER", user_id);
+    RESULT = new AlterTableSetOwnerStmt(table, new Owner(TOwnerType.USER, user));
+  :}
+  | KW_ALTER KW_TABLE table_name:table opt_partition_set:partitions KW_SET IDENT:owner_id
+    KW_ROLE ident_or_default:role
+  {:
+    // See above for special partition clause handling.
+    if (partitions != null) parser.parseError("set", SqlParserSymbols.KW_SET);
+    parser.checkIdentKeyword("OWNER", owner_id);
+    RESULT = new AlterTableSetOwnerStmt(table, new Owner(TOwnerType.ROLE, role));
+  :}
   ;
 
 table_property_type ::=
@@ -1877,6 +1895,18 @@ alter_view_stmt ::=
   {: RESULT = new AlterViewStmt(table, col_defs, view_def); :}
   | KW_ALTER KW_VIEW table_name:before_table KW_RENAME KW_TO table_name:new_table
   {: RESULT = new AlterTableOrViewRenameStmt(before_table, new_table, false); :}
+  | KW_ALTER KW_VIEW table_name:table KW_SET IDENT:owner_id IDENT:user_id
+    ident_or_default:user
+  {:
+    parser.checkIdentKeyword("OWNER", owner_id);
+    parser.checkIdentKeyword("USER", user_id);
+    RESULT = new AlterViewSetOwnerStmt(table, new Owner(TOwnerType.USER, user));
+  :}
+  | KW_ALTER KW_VIEW table_name:table KW_SET IDENT:owner_id KW_ROLE ident_or_default:role
+  {:
+    parser.checkIdentKeyword("OWNER", owner_id);
+    RESULT = new AlterViewSetOwnerStmt(table, new Owner(TOwnerType.ROLE, role));
+  :}
   ;
 
 cascade_val ::=

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
new file mode 100644
index 0000000..c508413
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
@@ -0,0 +1,72 @@
+// 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.impala.analysis;
+
+import com.google.common.base.Preconditions;
+import org.apache.impala.authorization.Privilege;
+import org.apache.impala.common.AnalysisException;
+import org.apache.impala.thrift.TAlterTableOrViewSetOwnerParams;
+import org.apache.impala.thrift.TAlterTableParams;
+import org.apache.impala.thrift.TAlterTableType;
+import org.apache.impala.util.MetaStoreUtil;
+
+/**
+ * A base class for ALTER TABLE/VIEW SET OWNER.
+ */
+public abstract class AlterTableOrViewSetOwnerStmt extends AlterTableStmt {
+  protected final Owner owner_;
+
+  public AlterTableOrViewSetOwnerStmt(TableName tableName, Owner owner) {
+    super(tableName);
+    Preconditions.checkNotNull(owner);
+    owner_ = owner;
+  }
+
+  @Override
+  public void analyze(Analyzer analyzer) throws AnalysisException {
+    String ownerName = owner_.getOwnerName();
+    if (ownerName.length() > MetaStoreUtil.MAX_OWNER_LENGTH) {
+      throw new AnalysisException(String.format("Owner name exceeds maximum length of " +
+          "%d characters. The given owner name has %d characters.",
+          MetaStoreUtil.MAX_OWNER_LENGTH, ownerName.length()));
+    }
+    tableName_ = analyzer.getFqTableName(tableName_);
+    TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER);
+    tableRef = analyzer.resolveTableRef(tableRef);
+    Preconditions.checkNotNull(tableRef);
+    tableRef.analyze(analyzer);
+    validateType(tableRef);
+  }
+
+  /**
+   * Validates the type of the given TableRef.
+   */
+  protected abstract void validateType(TableRef tableRef) throws AnalysisException;
+
+  @Override
+  public TAlterTableParams toThrift() {
+    TAlterTableParams params = new TAlterTableParams();
+    params.setTable_name(tableName_.toThrift());
+    TAlterTableOrViewSetOwnerParams ownerParams = new TAlterTableOrViewSetOwnerParams();
+    ownerParams.setOwner_type(owner_.getOwnerType());
+    ownerParams.setOwner_name(owner_.getOwnerName());
+    params.setAlter_type(TAlterTableType.SET_OWNER);
+    params.setSet_owner_params(ownerParams);
+    return params;
+  }
+}

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java
new file mode 100644
index 0000000..5932aff
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java
@@ -0,0 +1,37 @@
+// 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.impala.analysis;
+
+import org.apache.impala.common.AnalysisException;
+
+/**
+ * Represents an ALTER TABLE tbl SET OWNER [USER|ROLE] owner statement.
+ */
+public class AlterTableSetOwnerStmt extends AlterTableOrViewSetOwnerStmt {
+  public AlterTableSetOwnerStmt(TableName tableName, Owner owner) {
+    super(tableName, owner);
+  }
+
+  @Override
+  protected void validateType(TableRef tableRef) throws AnalysisException {
+    if (tableRef instanceof InlineViewRef) {
+      throw new AnalysisException(String.format(
+          "ALTER TABLE not allowed on a view: %s", tableName_));
+    }
+  }
+}
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
index a173975..0089f0d 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
@@ -33,7 +33,7 @@ import com.google.common.base.Preconditions;
  * Base class for all ALTER TABLE statements.
  */
 public abstract class AlterTableStmt extends StatementBase {
-  protected final TableName tableName_;
+  protected TableName tableName_;
 
   // Set during analysis.
   protected FeTable table_;

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java
new file mode 100644
index 0000000..4e1fc52
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java
@@ -0,0 +1,37 @@
+// 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.impala.analysis;
+
+import org.apache.impala.common.AnalysisException;
+
+/**
+ * Represents an ALTER VIEW v SET OWNER [USER|ROLE] owner statement.
+ */
+public class AlterViewSetOwnerStmt extends AlterTableOrViewSetOwnerStmt {
+  public AlterViewSetOwnerStmt(TableName tableName, Owner owner) {
+    super(tableName, owner);
+  }
+
+  @Override
+  protected void validateType(TableRef tableRef) throws AnalysisException {
+    if (!(tableRef instanceof InlineViewRef)) {
+      throw new AnalysisException(String.format(
+          "ALTER VIEW not allowed on a table: %s", tableName_));
+    }
+  }
+}

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
----------------------------------------------------------------------
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 0ce5d29..88668d7 100644
--- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
@@ -100,6 +100,7 @@ import org.apache.impala.thrift.TAlterTableAddReplaceColsParams;
 import org.apache.impala.thrift.TAlterTableAlterColParams;
 import org.apache.impala.thrift.TAlterTableDropColParams;
 import org.apache.impala.thrift.TAlterTableDropPartitionParams;
+import org.apache.impala.thrift.TAlterTableOrViewSetOwnerParams;
 import org.apache.impala.thrift.TAlterTableParams;
 import org.apache.impala.thrift.TAlterTableSetCachedParams;
 import org.apache.impala.thrift.TAlterTableSetFileFormatParams;
@@ -557,6 +558,11 @@ public class CatalogOpExecutor {
           alterTableRecoverPartitions(tbl);
           addSummary(response, "Partitions have been recovered.");
           break;
+        case SET_OWNER:
+          Preconditions.checkState(params.isSetSet_owner_params());
+          alterTableOrViewSetOwner(tbl, params.getSet_owner_params());
+          addSummary(response, "Updated table/view.");
+          break;
         default:
           throw new UnsupportedOperationException(
               "Unknown ALTER TABLE operation type: " + params.getAlter_type());
@@ -2781,6 +2787,14 @@ public class CatalogOpExecutor {
     }
   }
 
+  private void alterTableOrViewSetOwner(Table tbl, TAlterTableOrViewSetOwnerParams params)
+      throws ImpalaRuntimeException {
+    org.apache.hadoop.hive.metastore.api.Table msTbl = tbl.getMetaStoreTable().deepCopy();
+    msTbl.setOwner(params.owner_name);
+    msTbl.setOwnerType(PrincipalType.valueOf(params.owner_type.name()));
+    applyAlterTable(msTbl, true);
+  }
+
   /**
    * Create a new HMS Partition.
    */
@@ -3546,7 +3560,7 @@ public class CatalogOpExecutor {
       }
     }
     addDbToCatalogUpdate(db, response.result);
-    addSummary(response, "Updated database");
+    addSummary(response, "Updated database.");
   }
 
   private void addDbToCatalogUpdate(Db db, TCatalogUpdateResult result) {

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
index c565b45..46b3787 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -4059,6 +4059,44 @@ public class AnalyzeDDLTest extends FrontendTestBase {
     }
   }
 
+  @Test
+  public void TestAlterTableSetOwner() {
+    String[] ownerTypes = new String[]{"user", "role"};
+    for (String ownerType : ownerTypes) {
+      AnalyzesOk(String.format("alter table functional.alltypes set owner %s foo",
+          ownerType));
+      AnalysisError(String.format("alter table nodb.alltypes set owner %s foo",
+          ownerType), "Could not resolve table reference: 'nodb.alltypes'");
+      AnalysisError(String.format("alter table functional.notbl set owner %s foo",
+          ownerType), "Could not resolve table reference: 'functional.notbl'");
+      AnalysisError(String.format("alter table functional.alltypes set owner %s %s",
+          ownerType, buildLongOwnerName()), "Owner name exceeds maximum length of 128 " +
+          "characters. The given owner name has 133 characters.");
+      AnalysisError(String.format("alter table functional.alltypes_view " +
+          "set owner %s foo", ownerType), "ALTER TABLE not allowed on a view: " +
+          "functional.alltypes_view");
+    }
+  }
+
+  @Test
+  public void TestAlterViewSetOwner() {
+    String[] ownerTypes = new String[]{"user", "role"};
+    for (String ownerType : ownerTypes) {
+      AnalyzesOk(String.format("alter view functional.alltypes_view set owner %s foo",
+          ownerType));
+      AnalysisError(String.format("alter view nodb.alltypes set owner %s foo",
+          ownerType), "Could not resolve table reference: 'nodb.alltypes'");
+      AnalysisError(String.format("alter view functional.notbl set owner %s foo",
+          ownerType), "Could not resolve table reference: 'functional.notbl'");
+      AnalysisError(String.format("alter view functional.alltypes_view set owner %s %s",
+          ownerType, buildLongOwnerName()), "Owner name exceeds maximum length of 128 " +
+          "characters. The given owner name has 133 characters.");
+      AnalysisError(String.format("alter view functional.alltypes " +
+          "set owner %s foo", ownerType), "ALTER VIEW not allowed on a table: " +
+          "functional.alltypes");
+    }
+  }
+
   private static String buildLongOwnerName() {
     StringBuilder comment = new StringBuilder();
     for (int i = 0; i < MetaStoreUtil.MAX_OWNER_LENGTH + 5; i++) {

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
index 4f88463..d124701 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
@@ -1651,7 +1651,9 @@ public class AuthorizationStmtTest extends FrontendTestBase {
             "delimited fields terminated by ' '"),
         authorize("alter table functional.alltypes add partition(year=1, month=1)"),
         authorize("alter table functional.alltypes drop partition(" +
-            "year=2009, month=1)")}) {
+            "year=2009, month=1)"),
+        authorize("alter table functional.alltypes set owner user foo_owner"),
+        authorize("alter table functional.alltypes set owner role foo_owner")}) {
       test.ok(onServer(TPrivilegeLevel.ALL))
           .ok(onServer(TPrivilegeLevel.ALTER))
           .ok(onDatabase("functional", TPrivilegeLevel.ALL))
@@ -1824,6 +1826,25 @@ public class AuthorizationStmtTest extends FrontendTestBase {
         .error(alterError("functional.alltypes_view"), onTable("functional",
             "alltypes_view", allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
 
+    // Alter view set owner.
+    for (AuthzTest test: new AuthzTest[]{
+        authorize("alter view functional.alltypes_view set owner user foo_owner"),
+        authorize("alter view functional.alltypes_view set owner role foo_owner")}) {
+      test.ok(onServer(TPrivilegeLevel.ALL))
+          .ok(onServer(TPrivilegeLevel.ALTER))
+          .ok(onDatabase("functional", TPrivilegeLevel.ALL))
+          .ok(onDatabase("functional", TPrivilegeLevel.ALTER))
+          .ok(onTable("functional", "alltypes_view", TPrivilegeLevel.ALL))
+          .ok(onTable("functional", "alltypes_view", TPrivilegeLevel.ALTER))
+          .error(alterError("functional.alltypes_view"))
+          .error(alterError("functional.alltypes_view"), onServer(allExcept(
+              TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+          .error(alterError("functional.alltypes_view"), onDatabase("functional", allExcept(
+              TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+          .error(alterError("functional.alltypes_view"), onTable("functional",
+              "alltypes_view", allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
+    }
+
     // Database does not exist.
     authorize("alter view nodb.alltypes_view as select 1")
         .error(alterError("nodb"))

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
index 6059b8b..a86f467 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
@@ -3809,4 +3809,30 @@ public class ParserTest extends FrontendTestBase {
     ParserError("ALTER DATABASE SET OWNER ROLE foo");
     ParserError("ALTER DATABASE SET OWNER");
   }
+
+  @Test
+  public void TestAlterTableOrViewSetOwner() {
+    for (String type : new String[]{"TABLE", "VIEW"}) {
+      for (String valid : new String[]{"foo", "user", "owner"}) {
+        ParsesOk(String.format("ALTER %s %s SET OWNER USER %s", type, valid, valid));
+        ParsesOk(String.format("ALTER %s %s SET OWNER ROLE %s", type, valid, valid));
+      }
+
+      for (String invalid : new String[]{"'foo'", "''", "NULL"}) {
+        ParserError(String.format("ALTER %s %s SET OWNER ROLE %s", type, invalid, invalid));
+        ParserError(String.format("ALTER %s %s SET OWNER USER %s", type, invalid, invalid));
+      }
+
+      ParserError(String.format("ALTER %s tbl PARTITION(i=1) SET OWNER ROLE foo", type));
+      ParserError(String.format("ALTER %s tbl SET ABC USER foo", type));
+      ParserError(String.format("ALTER %s tbl SET ABC ROLE foo", type));
+      ParserError(String.format("ALTER %s tbl SET OWNER ABC foo", type));
+      ParserError(String.format("ALTER %s tbl SET OWNER USER", type));
+      ParserError(String.format("ALTER %s SET OWNER foo", type));
+      ParserError(String.format("ALTER %s SET OWNER USER foo", type));
+      ParserError(String.format("ALTER %s tbl SET OWNER ROLE", type));
+      ParserError(String.format("ALTER %s SET OWNER ROLE foo", type));
+      ParserError(String.format("ALTER %s SET OWNER", type));
+    }
+  }
 }

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/tests/metadata/test_ddl.py
----------------------------------------------------------------------
diff --git a/tests/metadata/test_ddl.py b/tests/metadata/test_ddl.py
index 7d2b714..ff6a566 100644
--- a/tests/metadata/test_ddl.py
+++ b/tests/metadata/test_ddl.py
@@ -227,6 +227,28 @@ class TestDdlStatements(TestDdlBase):
     assert len(properties) == 1
     assert {'foo_role': 'ROLE'} == properties
 
+  def test_alter_table_set_owner(self, vector, unique_database):
+    table_name = "{0}.test_owner_tbl".format(unique_database)
+    self.client.execute("create table {0}(i int)".format(table_name))
+    self.client.execute("alter table {0} set owner user foo_user".format(table_name))
+    owner = self._get_table_or_view_owner(table_name)
+    assert ('foo_user', 'USER') == owner
+
+    self.client.execute("alter table {0} set owner role foo_role".format(table_name))
+    owner = self._get_table_or_view_owner(table_name)
+    assert ('foo_role', 'ROLE') == owner
+
+  def test_alter_view_set_owner(self, vector, unique_database):
+    view_name = "{0}.test_owner_tbl".format(unique_database)
+    self.client.execute("create view {0} as select 1".format(view_name))
+    self.client.execute("alter view {0} set owner user foo_user".format(view_name))
+    owner = self._get_table_or_view_owner(view_name)
+    assert ('foo_user', 'USER') == owner
+
+    self.client.execute("alter view {0} set owner role foo_role".format(view_name))
+    owner = self._get_table_or_view_owner(view_name)
+    assert ('foo_role', 'ROLE') == owner
+
   # There is a query in QueryTest/create-table that references nested types, which is not
   # supported if old joins and aggs are enabled. Since we do not get any meaningful
   # additional coverage by running a DDL test under the old aggs and joins, it can be

http://git-wip-us.apache.org/repos/asf/impala/blob/54b3c607/tests/metadata/test_ddl_base.py
----------------------------------------------------------------------
diff --git a/tests/metadata/test_ddl_base.py b/tests/metadata/test_ddl_base.py
index bc74e6e..a27aa1c 100644
--- a/tests/metadata/test_ddl_base.py
+++ b/tests/metadata/test_ddl_base.py
@@ -93,6 +93,18 @@ class TestDdlBase(ImpalaTestSuite):
         properties[row[1].rstrip()] = row[2].rstrip()
     return properties
 
+  def _get_property(self, property_name, name, is_db=False):
+    """Extracts a db/table property value from the output of DESCRIBE FORMATTED."""
+    result = self.client.execute("describe {0} formatted {1}".format(
+      "database" if is_db else "", name))
+    for row in result.data:
+      if property_name in row:
+        row = row.split('\t')
+        if row[1] == 'NULL':
+          break
+        return row[1].rstrip()
+    return None
+
   def _get_db_comment(self, db_name):
     """Extracts the DB comment from the output of DESCRIBE DATABASE"""
     result = self.client.execute("describe database {0}".format(db_name))
@@ -110,3 +122,10 @@ class TestDdlBase(ImpalaTestSuite):
       if len(cols) == 3:
         comments[cols[0].rstrip()] = cols[2].rstrip()
     return comments.get(col_name)
+
+
+  def _get_table_or_view_owner(self, table_name):
+    """Returns a tuple(owner, owner_type) for a given table name"""
+    owner_name = self._get_property("Owner:", table_name)
+    owner_type = self._get_property("OwnerType:", table_name)
+    return (owner_name, owner_type)


[5/6] impala git commit: IMPALA-6918: [DOCS] COMMENT ON COLUMN privileges

Posted by ar...@apache.org.
IMPALA-6918: [DOCS] COMMENT ON COLUMN privileges

Change-Id: Ib2b3621ecc8113a165ec36beaf82cbc1d480bc54
Reviewed-on: http://gerrit.cloudera.org:8080/10890
Reviewed-by: Alex Rodoni <ar...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/2fddd39f
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/2fddd39f
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/2fddd39f

Branch: refs/heads/master
Commit: 2fddd39f0eb8cb4d476f35d1fe68f481444eb19b
Parents: a70c05e
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Mon Jul 9 09:23:18 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Jul 9 17:10:15 2018 +0000

----------------------------------------------------------------------
 docs/shared/impala_common.xml | 5 +++++
 1 file changed, 5 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/2fddd39f/docs/shared/impala_common.xml
----------------------------------------------------------------------
diff --git a/docs/shared/impala_common.xml b/docs/shared/impala_common.xml
index 4dcfffb..9617145 100644
--- a/docs/shared/impala_common.xml
+++ b/docs/shared/impala_common.xml
@@ -390,6 +390,11 @@ under the License.
                 <entry>TABLE</entry>
               </row>
               <row>
+                <entry>COMMENT ON COLUMN</entry>
+                <entry>ALTER</entry>
+                <entry>TABLE</entry>
+              </row>
+              <row>
                 <entry>DESCRIBE DATABASE</entry>
                 <entry>SELECT, INSERT, <b><i>or</i></b> REFRESH</entry>
                 <entry>DATABASE</entry>


[4/6] impala git commit: IMPALA-3956: [DOCS] Escape variables with '\' in impala-shell

Posted by ar...@apache.org.
IMPALA-3956: [DOCS]  Escape variables with '\' in impala-shell

Change-Id: Ifb95785a143939a94d55d3565364afe1e26c1f3d
Reviewed-on: http://gerrit.cloudera.org:8080/10861
Reviewed-by: Adam Holley <ah...@cloudera.com>
Reviewed-by: Fredy Wijaya <fw...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


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

Branch: refs/heads/master
Commit: a70c05ee2d295669707a2ab69d632f5b9cfab1cf
Parents: 880011f
Author: Alex Rodoni <ar...@cloudera.com>
Authored: Tue Jul 3 09:32:38 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Jul 9 16:39:43 2018 +0000

----------------------------------------------------------------------
 docs/topics/impala_shell_commands.xml         |  14 +-
 docs/topics/impala_shell_running_commands.xml | 397 ++++++---------------
 2 files changed, 120 insertions(+), 291 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/a70c05ee/docs/topics/impala_shell_commands.xml
----------------------------------------------------------------------
diff --git a/docs/topics/impala_shell_commands.xml b/docs/topics/impala_shell_commands.xml
index 91c6d13..f957f2f 100644
--- a/docs/topics/impala_shell_commands.xml
+++ b/docs/topics/impala_shell_commands.xml
@@ -260,12 +260,14 @@ under the License.
                 or <codeph>profile</codeph>.
               </p>
               <p>
-                Specify an integer argument. A positive integer <codeph>N</codeph>
-                represents the command labelled <codeph>N</codeph> in the history list.
-                A negative integer <codeph>-N</codeph> represents the <codeph>N</codeph>th
-                command from the end of the list, such as -1 for the most recent command.
-                Commands that are executed again do not produce new entries in the
-                history list.
+                Specify an integer argument. A positive integer
+                  <codeph>N</codeph> represents the command labelled
+                  <codeph>N</codeph> in the output of the
+                  <codeph>HISTORY</codeph> command. A negative integer
+                  <codeph>-N</codeph> represents the <codeph>N</codeph>th
+                command from the end of the list, such as -1 for the most recent
+                command. Commands that are executed again do not produce new
+                entries in the <codeph>HISTORY</codeph> output list.
               </p>
             </entry>
           </row>

http://git-wip-us.apache.org/repos/asf/impala/blob/a70c05ee/docs/topics/impala_shell_running_commands.xml
----------------------------------------------------------------------
diff --git a/docs/topics/impala_shell_running_commands.xml b/docs/topics/impala_shell_running_commands.xml
index 75a0758..519d705 100644
--- a/docs/topics/impala_shell_running_commands.xml
+++ b/docs/topics/impala_shell_running_commands.xml
@@ -21,7 +21,13 @@ under the License.
 <concept id="shell_running_commands">
 
   <title>Running Commands and SQL Statements in impala-shell</title>
-  <titlealts audience="PDF"><navtitle>Running Commands and SQL Statements</navtitle></titlealts>
+
+  <titlealts audience="PDF">
+
+    <navtitle>Running Commands and SQL Statements</navtitle>
+
+  </titlealts>
+
   <prolog>
     <metadata>
       <data name="Category" value="Impala"/>
@@ -35,314 +41,135 @@ under the License.
   <conbody>
 
     <p>
-      For information on available commands, see
-      <xref href="impala_shell_commands.xml#shell_commands"/>. You can see the full set of available
-      commands by pressing TAB twice, for example:
+      The following are a few of the key syntax and usage rules for running commands and SQL
+      statements in <codeph>impala-shell</codeph>.
     </p>
 
-<codeblock>[impalad-host:21000] &gt;
-connect   describe  explain   help      history   insert    quit      refresh   select    set       shell     show      use       version
-[impalad-host:21000] &gt;</codeblock>
+    <ul>
+      <li>
+        To see the full set of available commands, press <b>TAB</b> twice.
+      </li>
+
+      <li>
+        To cycle through and edit previous commands, click the up-arrow and down-arrow keys.
+      </li>
+
+      <li>
+        Use the standard set of keyboard shortcuts in GNU Readline library for editing and
+        cursor movement, such as <b>Ctrl-A</b> for the beginning of line and <b>Ctrl-E</b> for
+        the end of line.
+      </li>
+
+      <li>
+        Commands and SQL statements must be terminated by a semi-colon.
+      </li>
+
+      <li>
+        Commands and SQL statements can span multiple lines.
+      </li>
+
+      <li>
+        Use <codeph>--</codeph> to denote a single-line comment and /* */ to
+        denote a multi-line comment.
+
+        <p> A comment is considered part of the
+          statement it precedes, so when you enter a <codeph>--</codeph> or
+            <codeph>/* */</codeph> comment, you get a continuation prompt until
+          you finish entering a statement ending with a semicolon. For example:
+        </p>
+<codeblock>[impala] > -- This is a test comment
+                  > SHOW TABLES LIKE 't*';
+</codeblock>
+      </li>
 
-    <note>
-      Commands must be terminated by a semi-colon. A command can span multiple lines.
-    </note>
+      <li>
+        If a comment contains the
+          <codeph>${<varname>variable_name</varname>}</codeph> character and it
+        is not for a variable substitution, the <codeph>$</codeph> character
+        must be escaped, e.g. <codeph>-- \${hello}</codeph>.
+      </li>
+    </ul>
 
     <p>
-      For example:
+      For information on available <codeph>impala-shell</codeph> commands, see
+      <xref href="impala_shell_commands.xml#shell_commands"/>.
     </p>
 
-<codeblock>[localhost:21000] &gt; select *
-                  &gt; from t1
-                  &gt; limit 5;
-+---------+-----------+
-| s1      | s2        |
-+---------+-----------+
-| hello   | world     |
-| goodbye | cleveland |
-+---------+-----------+
-</codeblock>
+  </conbody>
 
-    <p>
-      A comment is considered part of the statement it precedes, so when you enter a <codeph>--</codeph> or
-      <codeph>/* */</codeph> comment, you get a continuation prompt until you finish entering a statement ending
-      with a semicolon:
-    </p>
+  <concept id="impala-shell_variable">
 
-<codeblock>[localhost:21000] &gt; -- This is a test comment
-                  &gt; show tables like 't*';
-+--------+
-| name   |
-+--------+
-| t1     |
-| t2     |
-| tab1   |
-| tab2   |
-| tab3   |
-| text_t |
-+--------+
-</codeblock>
+    <title>Variable Substitution in impala-shell</title>
 
-    <p>
-      Use the up-arrow and down-arrow keys to cycle through and edit previous commands.
-      <cmdname>impala-shell</cmdname> uses the <codeph>readline</codeph> library and so supports a standard set of
-      keyboard shortcuts for editing and cursor movement, such as <codeph>Ctrl-A</codeph> for beginning of line and
-      <codeph>Ctrl-E</codeph> for end of line.
-    </p>
+    <conbody>
 
-    <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
-      In <keyword keyref="impala25_full"/> and higher, you can define substitution variables to be used within SQL statements
-      processed by <cmdname>impala-shell</cmdname>. On the command line, you specify the option
-      <codeph>--var=<varname>variable_name</varname>=<varname>value</varname></codeph>.
-      Within an interactive session or a script file processed by the <codeph>-f</codeph> option, you specify
-      a <codeph>SET</codeph> command using the notation <codeph>SET VAR:<varname>variable_name</varname>=<varname>value</varname></codeph>.
-      Within a SQL statement, you substitute the value by using the notation <codeph>${var:<varname>variable_name</varname>}</codeph>.
-    </p>
+      <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
+        In <keyword keyref="impala25_full"/> and higher, you can define substitution variables
+        to be used within SQL statements processed by <cmdname>impala-shell</cmdname>.
+        <ol>
+          <li>
+            You specify the variable and its value as below.
+            <ul>
+              <li>
+                On the command line, you specify the option
+                <codeph>--var=<varname>variable_name</varname>=<varname>value</varname></codeph>
+              </li>
+
+              <li>
+                Within an interactive session or a script file processed by the
+                <codeph>-f</codeph> option, use the <codeph>SET
+                VAR:<varname>variable_name</varname>=<varname>value</varname></codeph> command.
+              </li>
+            </ul>
+          </li>
+
+          <li>
+            Use the above variable in SQL statements in the <codeph>impala-shell</codeph>
+            session using the notation:
+            <codeph>${VAR:<varname>variable_name</varname>}</codeph>.
+          </li>
+        </ol>
+      </p>
 
-    <note>
-      Because this feature is part of <cmdname>impala-shell</cmdname> rather than the <cmdname>impalad</cmdname>
-      backend, make sure the client system you are connecting from has the most recent <cmdname>impala-shell</cmdname>.
-      You can use this feature with a new <cmdname>impala-shell</cmdname> connecting to an older <cmdname>impalad</cmdname>,
-      but not the reverse.
-    </note>
-
-    <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
-      For example, here are some <cmdname>impala-shell</cmdname> commands that define substitution variables and then
-      use them in SQL statements executed through the <codeph>-q</codeph> and <codeph>-f</codeph> options.
-      Notice how the <codeph>-q</codeph> argument strings are single-quoted to prevent shell expansion of the
-      <codeph>${var:value}</codeph> notation, and any string literals within the queries are enclosed by double quotation marks.
-    </p>
+      <note>
+        Because this feature is part of <cmdname>impala-shell</cmdname> rather than the
+        <cmdname>impalad</cmdname> backend, make sure the client system you are connecting from
+        has the most recent <cmdname>impala-shell</cmdname>. You can use this feature with a new
+        <cmdname>impala-shell</cmdname> connecting to an older <cmdname>impalad</cmdname>, but
+        not the reverse.
+      </note>
+
+      <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
+        For example, here are some <cmdname>impala-shell</cmdname> commands that define
+        substitution variables and then use them in SQL statements executed through the
+        <codeph>-q</codeph> and <codeph>-f</codeph> options. Notice how the <codeph>-q</codeph>
+        argument strings are single-quoted to prevent shell expansion of the
+        <codeph>${var:value}</codeph> notation, and any string literals within the queries are
+        enclosed by double quotation marks.
+      </p>
 
 <codeblock rev="2.5.0 IMPALA-2179 IMPALA-2180">
-$ impala-shell --var=tname=table1 --var=colname=x --var=coltype=string -q 'create table ${var:tname} (${var:colname} ${var:coltype}) stored as parquet'
-Starting Impala Shell without Kerberos authentication
-Connected to <varname>hostname</varname>
-Server version: <varname>impalad_version</varname>
-Query: create table table1 (x string) stored as parquet
-
-$ NEW_STRING="hello world"
-$ impala-shell --var=tname=table1 --var=insert_val="$NEW_STRING" -q 'insert into ${var:tname} values ("${var:insert_val}")'
-Starting Impala Shell without Kerberos authentication
-Connected to <varname>hostname</varname>
-Server version: <varname>impalad_version</varname>
-Query: insert into table1 values ("hello world")
-Inserted 1 row(s) in 1.40s
-
-$ for VAL in foo bar bletch
-do
-  impala-shell --var=tname=table1 --var=insert_val="$VAL" -q 'insert into ${var:tname} values ("${var:insert_val}")'
-done
-...
-Query: insert into table1 values ("foo")
-Inserted 1 row(s) in 0.22s
-Query: insert into table1 values ("bar")
-Inserted 1 row(s) in 0.11s
-Query: insert into table1 values ("bletch")
-Inserted 1 row(s) in 0.21s
-
-$ echo "Search for what substring?" ; read answer
-Search for what substring?
-b
-$ impala-shell --var=tname=table1 -q 'select x from ${var:tname} where x like "%${var:answer}%"'
-Starting Impala Shell without Kerberos authentication
-Connected to <varname>hostname</varname>
-Server version: <varname>impalad_version</varname>
-Query: select x from table1 where x like "%b%"
-+--------+
-| x      |
-+--------+
-| bletch |
-| bar    |
-+--------+
-Fetched 2 row(s) in 0.83s
+$ impala-shell --var=tname=table1 --var=colname=x --var=coltype=string -q 'CREATE TABLE ${var:tname} (${var:colname} ${var:coltype}) STORED AS PARQUET'
+Query: CREATE TABLE table1 (x STRING) STORED AS PARQUET
 </codeblock>
 
-    <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
-      Here is a substitution variable passed in by the <codeph>--var</codeph> option,
-      and then referenced by statements issued interactively. Then the variable is
-      cleared with the <codeph>UNSET</codeph> command, and defined again with the
-      <codeph>SET</codeph> command.
-    </p>
+      <p rev="2.5.0 IMPALA-2179 IMPALA-2180">
+        The below example shows a substitution variable passed in by the <codeph>--var</codeph>
+        option, and then referenced by statements issued interactively. Then the variable is
+        reset with the <codeph>SET</codeph> command.
+      </p>
 
 <codeblock rev="2.5.0 IMPALA-2179 IMPALA-2180">
 $ impala-shell --quiet --var=tname=table1
-Starting Impala Shell without Kerberos authentication
-***********************************************************************************
-<varname>banner_message</varname>
-***********************************************************************************
-[<varname>hostname</varname>:21000] > select count(*) from ${var:tname};
-+----------+
-| count(*) |
-+----------+
-| 4        |
-+----------+
-[<varname>hostname</varname>:21000] > unset var:tname;
-Unsetting variable TNAME
-[<varname>hostname</varname>:21000] > select count(*) from ${var:tname};
-Error: Unknown variable TNAME
-[<varname>hostname</varname>:21000] > set var:tname=table1;
-[<varname>hostname</varname>:21000] > select count(*) from ${var:tname};
-+----------+
-| count(*) |
-+----------+
-| 4        |
-+----------+
-</codeblock>
 
-    <p rev="IMPALA-3397">
-      The following example shows how the <codeph>SOURCE</codeph> command can execute
-      a series of statements from a file:
-    </p>
+[impala] > SELECT COUNT(*) FROM ${var:tname};
 
-<codeblock rev="IMPALA-3397">
-$ cat commands.sql
-show databases;
-show tables in default;
-show functions in _impala_builtins like '*minute*';
-
-$ impala-shell -i localhost
-...
-[localhost:21000] > source commands.sql;
-Query: show databases
-+------------------+----------------------------------------------+
-| name             | comment                                      |
-+------------------+----------------------------------------------+
-| _impala_builtins | System database for Impala builtin functions |
-| default          | Default Hive database                        |
-+------------------+----------------------------------------------+
-Fetched 2 row(s) in 0.06s
-Query: show tables in default
-+-----------+
-| name      |
-+-----------+
-| customers |
-| sample_07 |
-| sample_08 |
-| web_logs  |
-+-----------+
-Fetched 4 row(s) in 0.02s
-Query: show functions in _impala_builtins like '*minute*'
-+-------------+--------------------------------+-------------+---------------+
-| return type | signature                      | binary type | is persistent |
-+-------------+--------------------------------+-------------+---------------+
-| INT         | minute(TIMESTAMP)              | BUILTIN     | true          |
-| TIMESTAMP   | minutes_add(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | minutes_add(TIMESTAMP, INT)    | BUILTIN     | true          |
-| TIMESTAMP   | minutes_sub(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | minutes_sub(TIMESTAMP, INT)    | BUILTIN     | true          |
-+-------------+--------------------------------+-------------+---------------+
-Fetched 5 row(s) in 0.03s
-</codeblock>
-
-    <p rev="IMPALA-3397">
-      The following example shows how a file that is run by the <codeph>SOURCE</codeph> command,
-      or through the <codeph>-q</codeph> or <codeph>-f</codeph> options of <cmdname>impala-shell</cmdname>,
-      can contain additional <codeph>SOURCE</codeph> commands.
-      The first file, <filepath>nested1.sql</filepath>, runs an <cmdname>impala-shell</cmdname> command
-      and then also runs the commands from <filepath>nested2.sql</filepath>.
-      This ability for scripts to call each other is often useful for code that sets up schemas for applications
-      or test environments.
-    </p>
-
-<codeblock rev="IMPALA-3397">
-$ cat nested1.sql
-show functions in _impala_builtins like '*minute*';
-source nested2.sql
-$ cat nested2.sql
-show functions in _impala_builtins like '*hour*'
-
-$ impala-shell -i localhost -f nested1.sql
-Starting Impala Shell without Kerberos authentication
-Connected to localhost:21000
-...
-Query: show functions in _impala_builtins like '*minute*'
-+-------------+--------------------------------+-------------+---------------+
-| return type | signature                      | binary type | is persistent |
-+-------------+--------------------------------+-------------+---------------+
-| INT         | minute(TIMESTAMP)              | BUILTIN     | true          |
-| TIMESTAMP   | minutes_add(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | minutes_add(TIMESTAMP, INT)    | BUILTIN     | true          |
-| TIMESTAMP   | minutes_sub(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | minutes_sub(TIMESTAMP, INT)    | BUILTIN     | true          |
-+-------------+--------------------------------+-------------+---------------+
-Fetched 5 row(s) in 0.01s
-Query: show functions in _impala_builtins like '*hour*'
-+-------------+------------------------------+-------------+---------------+
-| return type | signature                    | binary type | is persistent |
-+-------------+------------------------------+-------------+---------------+
-| INT         | hour(TIMESTAMP)              | BUILTIN     | true          |
-| TIMESTAMP   | hours_add(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | hours_add(TIMESTAMP, INT)    | BUILTIN     | true          |
-| TIMESTAMP   | hours_sub(TIMESTAMP, BIGINT) | BUILTIN     | true          |
-| TIMESTAMP   | hours_sub(TIMESTAMP, INT)    | BUILTIN     | true          |
-+-------------+------------------------------+-------------+---------------+
-Fetched 5 row(s) in 0.01s
-</codeblock>
-
-  </conbody>
-
-  <concept id="rerun" rev="2.10.0 IMPALA-992">
-    <title>Rerunning impala-shell Commands</title>
-    <conbody>
-
-      <p>
-        In <keyword keyref="impala210_full"/> and higher, you can use the
-        <codeph>rerun</codeph> command, or its abbreviation <codeph>@</codeph>,
-        to re-execute commands from the history list. The argument can be
-        a positive integer (reflecting the number shown in <codeph>history</codeph>
-        output) or a negative integer (reflecting the N'th last command in the
-        <codeph>history</codeph> output. For example:
-      </p>
-
-<codeblock><![CDATA[
-[localhost:21000] > select * from p1 order by t limit 5;
-...
-[localhost:21000] > show table stats p1;
-+-----------+--------+--------+------------------------------------------------------------+
-| #Rows     | #Files | Size   | Location                                                   |
-+-----------+--------+--------+------------------------------------------------------------+
-| 134217728 | 50     | 4.66MB | hdfs://test.example.com:8020/user/hive/warehouse/jdr.db/p1 |
-+-----------+--------+--------+------------------------------------------------------------+
-[localhost:21000] > compute stats p1;
-+-----------------------------------------+
-| summary                                 |
-+-----------------------------------------+
-| Updated 1 partition(s) and 3 column(s). |
-+-----------------------------------------+
-[localhost:21000] > history;
-[1]: use jdr;
-[2]: history;
-[3]: show tables;
-[4]: select * from p1 order by t limit 5;
-[5]: show table stats p1;
-[6]: compute stats p1;
-[7]: history;
-[localhost:21000] > @-2; <- Rerun the 2nd last command in the history list
-Rerunning compute stats p1;
-+-----------------------------------------+
-| summary                                 |
-+-----------------------------------------+
-| Updated 1 partition(s) and 3 column(s). |
-+-----------------------------------------+
-[localhost:21000] > history; <- History list is not updated by rerunning commands
-                                or by repeating the last command, in this case 'history'.
-[1]: use jdr;
-[2]: history;
-[3]: show tables;
-[4]: select * from p1 order by t limit 5;
-[5]: show table stats p1;
-[6]: compute stats p1;
-[7]: history;
-[localhost:21000] > @4; <- Rerun command #4 in the history list using short form '@'.
-Rerunning select * from p1 order by t limit 5;
-...
-[localhost:21000] > rerun 4; <- Rerun command #4 using long form 'rerun'.
-Rerunning select * from p1 order by t limit 5;
-...
-]]>
+[impala] > SET VAR:tname=table2;
+[impala] > SELECT COUNT(*) FROM ${var:tname};
 </codeblock>
 
     </conbody>
+
   </concept>
 
 </concept>


[6/6] impala git commit: [DOCS] Removed an unused keydef

Posted by ar...@apache.org.
[DOCS] Removed an unused keydef

Change-Id: I694adc75f8e1dbdf2913b18438f127f2d9dfd6b6
Reviewed-on: http://gerrit.cloudera.org:8080/10892
Reviewed-by: Alex Rodoni <ar...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/7a8f61c7
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/7a8f61c7
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/7a8f61c7

Branch: refs/heads/master
Commit: 7a8f61c79eb13eea8202b9a17cfa416d7e113e76
Parents: 2fddd39
Author: Alex Rodoni <ar...@cloudera.com>
Authored: Mon Jul 9 10:43:07 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Jul 9 17:50:52 2018 +0000

----------------------------------------------------------------------
 docs/impala_keydefs.ditamap | 1 -
 1 file changed, 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/7a8f61c7/docs/impala_keydefs.ditamap
----------------------------------------------------------------------
diff --git a/docs/impala_keydefs.ditamap b/docs/impala_keydefs.ditamap
index 056af35..0950af2 100644
--- a/docs/impala_keydefs.ditamap
+++ b/docs/impala_keydefs.ditamap
@@ -10914,7 +10914,6 @@ under the License.
   <keydef href="topics/impala_shell_options.xml#shell_config_file" keys="shell_config_file"/>
   <keydef href="topics/impala_connecting.xml" keys="connecting"/>
   <keydef href="topics/impala_shell_running_commands.xml" keys="shell_running_commands"/>
-  <keydef href="topics/impala_shell_running_commands.xml#rerun" keys="rerun"/>
   <keydef href="topics/impala_shell_commands.xml" keys="shell_commands"/>
 
   <keydef href="topics/impala_performance.xml" keys="performance"/>


[3/6] impala git commit: IMPALA-6031: Fix executor node count in distributed plans

Posted by ar...@apache.org.
IMPALA-6031: Fix executor node count in distributed plans

Prior to this change, the planner also considered coordinator-only
nodes as executors while estimating the number of scan nodes to be
used in the distributed plan. This change ensures that only
executor nodes are considered for that estimation.

Testing:
Added a new custom cluster test to verify the same.

Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8
Reviewed-on: http://gerrit.cloudera.org:8080/10873
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/880011fa
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/880011fa
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/880011fa

Branch: refs/heads/master
Commit: 880011fa1f2fc48f9972ad3c673a4bff838cc5ce
Parents: 2816211
Author: poojanilangekar <po...@cloudera.com>
Authored: Thu Jun 14 09:22:03 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Sat Jul 7 03:33:08 2018 +0000

----------------------------------------------------------------------
 be/src/service/frontend.cc                      |  4 +-
 be/src/service/frontend.h                       |  6 +-
 be/src/service/impala-server.cc                 |  9 ++-
 common/thrift/Frontend.thrift                   | 14 +++-
 .../apache/impala/planner/HBaseScanNode.java    |  9 +--
 .../org/apache/impala/planner/HdfsScanNode.java | 16 ++--
 .../org/apache/impala/service/Frontend.java     |  8 +-
 .../org/apache/impala/service/JniFrontend.java  | 11 +--
 .../impala/util/ExecutorMembershipSnapshot.java | 82 +++++++++++++++++++
 .../apache/impala/util/MembershipSnapshot.java  | 84 --------------------
 .../apache/impala/planner/PlannerTestBase.java  | 10 +--
 tests/custom_cluster/test_coordinators.py       | 36 +++++++++
 12 files changed, 165 insertions(+), 124 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/be/src/service/frontend.cc
----------------------------------------------------------------------
diff --git a/be/src/service/frontend.cc b/be/src/service/frontend.cc
index 9ae9f90..ca3f79f 100644
--- a/be/src/service/frontend.cc
+++ b/be/src/service/frontend.cc
@@ -81,7 +81,7 @@ Frontend::Frontend() {
     {"getHadoopGroups", "([B)[B", &get_hadoop_groups_id_},
     {"checkConfiguration", "()Ljava/lang/String;", &check_config_id_},
     {"updateCatalogCache", "([B)[B", &update_catalog_cache_id_},
-    {"updateMembership", "([B)V", &update_membership_id_},
+    {"updateExecutorMembership", "([B)V", &update_membership_id_},
     {"getCatalogMetrics", "()[B", &get_catalog_metrics_id_},
     {"getTableNames", "([B)[B", &get_table_names_id_},
     {"describeDb", "([B)[B", &describe_db_id_},
@@ -126,7 +126,7 @@ Status Frontend::UpdateCatalogCache(const TUpdateCatalogCacheRequest& req,
   return JniUtil::CallJniMethod(fe_, update_catalog_cache_id_, req, resp);
 }
 
-Status Frontend::UpdateMembership(const TUpdateMembershipRequest& req) {
+Status Frontend::UpdateExecutorMembership(const TUpdateExecutorMembershipRequest& req) {
   return JniUtil::CallJniMethod(fe_, update_membership_id_, req);
 }
 

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/be/src/service/frontend.h
----------------------------------------------------------------------
diff --git a/be/src/service/frontend.h b/be/src/service/frontend.h
index 77836ab..2327e8f 100644
--- a/be/src/service/frontend.h
+++ b/be/src/service/frontend.h
@@ -45,9 +45,9 @@ class Frontend {
   Status UpdateCatalogCache(const TUpdateCatalogCacheRequest& req,
       TUpdateCatalogCacheResponse *resp);
 
-  /// Request to update the Impalad frontend cluster membership snapshot.  The
-  /// TUpdateMembershipRequest contains the latest set of hosts.
-  Status UpdateMembership(const TUpdateMembershipRequest& req);
+  /// Request to update the Impalad frontend cluster membership snapshot of executors.
+  /// The TUpdateExecutorMembershipRequest contains the latest set of executor nodes.
+  Status UpdateExecutorMembership(const TUpdateExecutorMembershipRequest& req);
 
   /// Call FE to get explain plan
   Status GetExplainPlan(const TQueryCtx& query_ctx, std::string* explain_string);

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index c1f00e3..97fa70e 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -1667,18 +1667,19 @@ void ImpalaServer::MembershipCallback(
     // membership by network address.
     set<TNetworkAddress> current_membership;
     // Also reflect changes to the frontend. Initialized only if any_changes is true.
-    TUpdateMembershipRequest update_req;
+    // Only send the hostname and ip_address of the executors to the frontend.
+    TUpdateExecutorMembershipRequest update_req;
     bool any_changes = !delta.topic_entries.empty() || !delta.is_delta;
     for (const BackendDescriptorMap::value_type& backend: known_backends_) {
       current_membership.insert(backend.second.address);
-      if (any_changes) {
+      if (any_changes && backend.second.is_executor) {
         update_req.hostnames.insert(backend.second.address.hostname);
         update_req.ip_addresses.insert(backend.second.ip_address);
+        update_req.num_executors++;
       }
     }
     if (any_changes) {
-      update_req.num_nodes = known_backends_.size();
-      Status status = exec_env_->frontend()->UpdateMembership(update_req);
+      Status status = exec_env_->frontend()->UpdateExecutorMembership(update_req);
       if (!status.ok()) {
         LOG(WARNING) << "Error updating frontend membership snapshot: "
                      << status.GetDetail();

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/common/thrift/Frontend.thrift
----------------------------------------------------------------------
diff --git a/common/thrift/Frontend.thrift b/common/thrift/Frontend.thrift
index 6cbbf79..1245c94 100644
--- a/common/thrift/Frontend.thrift
+++ b/common/thrift/Frontend.thrift
@@ -711,12 +711,18 @@ struct TUpdateCatalogCacheResponse {
   3: required i64 new_catalog_version
 }
 
-// Sent from the impalad BE to FE with the latest cluster membership snapshot resulting
-// from the Membership heartbeat.
-struct TUpdateMembershipRequest {
+// Sent from the impalad BE to FE with the latest membership snapshot of the
+// executors on the cluster resulting from the Membership heartbeat.
+struct TUpdateExecutorMembershipRequest {
+  // The hostnames of the executor nodes.
   1: required set<string> hostnames
+
+  // The ip addresses of the executor nodes.
   2: required set<string> ip_addresses
-  3: i32 num_nodes
+
+  // The number of executors on a cluster, needed since there can be multiple
+  // impalads running on the same host.
+  3: i32 num_executors
 }
 
 // Contains all interesting statistics from a single 'memory pool' in the JVM.

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java b/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
index 881c4ae..5184f96 100644
--- a/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
+++ b/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
@@ -53,7 +53,7 @@ import org.apache.impala.thrift.TScanRange;
 import org.apache.impala.thrift.TScanRangeLocation;
 import org.apache.impala.thrift.TScanRangeLocationList;
 import org.apache.impala.thrift.TScanRangeSpec;
-import org.apache.impala.util.MembershipSnapshot;
+import org.apache.impala.util.ExecutorMembershipSnapshot;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -218,11 +218,10 @@ public class HBaseScanNode extends ScanNode {
       LOG.trace("computeStats HbaseScan: cardinality=" + Long.toString(cardinality_));
     }
 
-    // Assume that each node in the cluster gets a scan range, unless there are fewer
+    // Assume that each executor in the cluster gets a scan range, unless there are fewer
     // scan ranges than nodes.
-    numNodes_ = Math.max(1,
-        Math.min(scanRangeSpecs_.getConcrete_rangesSize(),
-            MembershipSnapshot.getCluster().numNodes()));
+    numNodes_ = Math.max(1, Math.min(scanRangeSpecs_.getConcrete_rangesSize(),
+                                ExecutorMembershipSnapshot.getCluster().numExecutors()));
     if (LOG.isTraceEnabled()) {
       LOG.trace("computeStats HbaseScan: #nodes=" + Integer.toString(numNodes_));
     }

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java b/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
index ac6c85a..55bf301 100644
--- a/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
+++ b/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
@@ -83,7 +83,7 @@ import org.apache.impala.thrift.TScanRangeLocationList;
 import org.apache.impala.thrift.TScanRangeSpec;
 import org.apache.impala.thrift.TTableStats;
 import org.apache.impala.util.BitUtil;
-import org.apache.impala.util.MembershipSnapshot;
+import org.apache.impala.util.ExecutorMembershipSnapshot;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -1100,7 +1100,7 @@ public class HdfsScanNode extends ScanNode {
    */
   protected void computeNumNodes(Analyzer analyzer, long cardinality) {
     Preconditions.checkNotNull(scanRangeSpecs_);
-    MembershipSnapshot cluster = MembershipSnapshot.getCluster();
+    ExecutorMembershipSnapshot cluster = ExecutorMembershipSnapshot.getCluster();
     HashSet<TNetworkAddress> localHostSet = Sets.newHashSet();
     int totalNodes = 0;
     int numLocalRanges = 0;
@@ -1135,21 +1135,21 @@ public class HdfsScanNode extends ScanNode {
         // hosts that hold block replica for those ranges.
         int numLocalNodes = Math.min(numLocalRanges, localHostSet.size());
         // The remote ranges are round-robined across all the impalads.
-        int numRemoteNodes = Math.min(numRemoteRanges, cluster.numNodes());
+        int numRemoteNodes = Math.min(numRemoteRanges, cluster.numExecutors());
         // The local and remote assignments may overlap, but we don't know by how much so
         // conservatively assume no overlap.
-        totalNodes = Math.min(numLocalNodes + numRemoteNodes, cluster.numNodes());
+        totalNodes = Math.min(numLocalNodes + numRemoteNodes, cluster.numExecutors());
         // Exit early if all hosts have a scan range assignment, to avoid extraneous work
         // in case the number of scan ranges dominates the number of nodes.
-        if (totalNodes == cluster.numNodes()) break;
+        if (totalNodes == cluster.numExecutors()) break;
       }
     }
     // Handle the generated range specifications.
-    if (totalNodes < cluster.numNodes() && scanRangeSpecs_.isSetSplit_specs()) {
+    if (totalNodes < cluster.numExecutors() && scanRangeSpecs_.isSetSplit_specs()) {
       Preconditions.checkState(
           generatedScanRangeCount_ >= scanRangeSpecs_.getSplit_specsSize());
       numRemoteRanges += generatedScanRangeCount_;
-      totalNodes = Math.min(numRemoteRanges, cluster.numNodes());
+      totalNodes = Math.min(numRemoteRanges, cluster.numExecutors());
     }
     // Tables can reside on 0 nodes (empty table), but a plan node must always be
     // executed on at least one node.
@@ -1159,7 +1159,7 @@ public class HdfsScanNode extends ScanNode {
           + (scanRangeSpecs_.getConcrete_rangesSize() + generatedScanRangeCount_)
           + " localRanges=" + numLocalRanges + " remoteRanges=" + numRemoteRanges
           + " localHostSet.size=" + localHostSet.size()
-          + " clusterNodes=" + cluster.numNodes());
+          + " executorNodes=" + cluster.numExecutors());
     }
   }
 

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/service/Frontend.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/Frontend.java b/fe/src/main/java/org/apache/impala/service/Frontend.java
index 9208184..a363458 100644
--- a/fe/src/main/java/org/apache/impala/service/Frontend.java
+++ b/fe/src/main/java/org/apache/impala/service/Frontend.java
@@ -136,9 +136,9 @@ import org.apache.impala.thrift.TStmtType;
 import org.apache.impala.thrift.TTableName;
 import org.apache.impala.thrift.TUpdateCatalogCacheRequest;
 import org.apache.impala.thrift.TUpdateCatalogCacheResponse;
-import org.apache.impala.thrift.TUpdateMembershipRequest;
+import org.apache.impala.thrift.TUpdateExecutorMembershipRequest;
 import org.apache.impala.util.EventSequence;
-import org.apache.impala.util.MembershipSnapshot;
+import org.apache.impala.util.ExecutorMembershipSnapshot;
 import org.apache.impala.util.PatternMatcher;
 import org.apache.impala.util.TResultRowBuilder;
 import org.apache.impala.util.TSessionStateUtil;
@@ -252,8 +252,8 @@ public class Frontend {
   /**
    * Update the cluster membership snapshot with the latest snapshot from the backend.
    */
-  public void updateMembership(TUpdateMembershipRequest req) {
-    MembershipSnapshot.update(req);
+  public void updateExecutorMembership(TUpdateExecutorMembershipRequest req) {
+    ExecutorMembershipSnapshot.update(req);
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/service/JniFrontend.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/JniFrontend.java b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
index ad8b165..f02db61 100644
--- a/fe/src/main/java/org/apache/impala/service/JniFrontend.java
+++ b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
@@ -95,7 +95,7 @@ import org.apache.impala.thrift.TShowStatsOp;
 import org.apache.impala.thrift.TShowStatsParams;
 import org.apache.impala.thrift.TTableName;
 import org.apache.impala.thrift.TUpdateCatalogCacheRequest;
-import org.apache.impala.thrift.TUpdateMembershipRequest;
+import org.apache.impala.thrift.TUpdateExecutorMembershipRequest;
 import org.apache.impala.util.GlogAppender;
 import org.apache.impala.util.PatternMatcher;
 import org.apache.impala.util.TSessionStateUtil;
@@ -186,12 +186,13 @@ public class JniFrontend {
 
   /**
    * Jni wrapper for Frontend.updateMembership(). Accepts a serialized
-   * TUpdateMembershipRequest.
+   * TUpdateExecutorMembershipRequest.
    */
-  public void updateMembership(byte[] thriftMembershipUpdate) throws ImpalaException {
-    TUpdateMembershipRequest req = new TUpdateMembershipRequest();
+  public void updateExecutorMembership(byte[] thriftMembershipUpdate)
+      throws ImpalaException {
+    TUpdateExecutorMembershipRequest req = new TUpdateExecutorMembershipRequest();
     JniUtil.deserializeThrift(protocolFactory_, req, thriftMembershipUpdate);
-    frontend_.updateMembership(req);
+    frontend_.updateExecutorMembership(req);
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java b/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
new file mode 100644
index 0000000..3482d6b
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
@@ -0,0 +1,82 @@
+// 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.impala.util;
+
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.impala.thrift.TNetworkAddress;
+import org.apache.impala.thrift.TUpdateExecutorMembershipRequest;
+import com.google.common.collect.Sets;
+
+/**
+ * Singleton class that represents a snapshot of the Impalad executor membership. Host
+ * membership is determined by both IP address and hostname (to mimic the backend's
+ * Scheduler). A new snapshot is created whenever the cluster membership changes
+ * so that clients don't need to hold a lock while examining a snapshot.
+ */
+public class ExecutorMembershipSnapshot {
+  // The latest instance of the ExecutorMembershipSnapshot.
+  private static AtomicReference<ExecutorMembershipSnapshot> cluster_ =
+      new AtomicReference<ExecutorMembershipSnapshot>(new ExecutorMembershipSnapshot());
+
+  // The set of hosts that are members of the cluster given by hostname.
+  private final Set<String> hostnames_;
+
+  // The set of hosts that are members of the cluster given by IP address.
+  private final Set<String> ipAddresses_;
+
+  // The number of executor nodes of the cluster.  Normally, this will be equal to
+  // hostnames_.size(), except in the test minicluster where there are multiple
+  // impalad's running on a single host.
+  private final int numExecutors_;
+
+  // Used only to construct the initial ExecutorMembershipSnapshot. Before we get the
+  // first snapshot, assume one node (the localhost) to mimic Scheduler.
+  private ExecutorMembershipSnapshot() {
+    hostnames_ = Sets.newHashSet();
+    ipAddresses_ = Sets.newHashSet();
+    numExecutors_ = 1;
+  }
+
+  // Construct a new snapshot based on the TUpdateExecutorMembershipRequest.
+  private ExecutorMembershipSnapshot(TUpdateExecutorMembershipRequest request) {
+    hostnames_ = request.getHostnames();
+    ipAddresses_ = request.getIp_addresses();
+    numExecutors_ = request.getNum_executors();
+  }
+
+  // Determine whether a host, given either by IP address or hostname, is a member of
+  // this snapshot.  Returns true if it is, false otherwise.
+  public boolean contains(TNetworkAddress address) {
+    String host = address.getHostname();
+    return ipAddresses_.contains(host) || hostnames_.contains(host);
+  }
+
+  // The number of nodes in this snapshot.
+  public int numExecutors() { return numExecutors_; }
+
+  // Atomically update the singleton snapshot instance.  After the update completes,
+  // all calls to getCluster() will return the new snapshot.
+  public static void update(TUpdateExecutorMembershipRequest request) {
+    cluster_.set(new ExecutorMembershipSnapshot(request));
+  }
+
+  // Return the current singleton snapshot instance.
+  public static ExecutorMembershipSnapshot getCluster() { return cluster_.get(); }
+}

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/main/java/org/apache/impala/util/MembershipSnapshot.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/util/MembershipSnapshot.java b/fe/src/main/java/org/apache/impala/util/MembershipSnapshot.java
deleted file mode 100644
index be44785..0000000
--- a/fe/src/main/java/org/apache/impala/util/MembershipSnapshot.java
+++ /dev/null
@@ -1,84 +0,0 @@
-// 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.impala.util;
-
-import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
-
-import org.apache.impala.thrift.TNetworkAddress;
-import org.apache.impala.thrift.TUpdateMembershipRequest;
-import com.google.common.collect.Sets;
-
-/**
- * Singleton class that represents a snapshot of the Impalad cluster membership.  Host
- * membership is determined by both IP address and hostname (to mimic the backend's
- * Scheduler).  A new snapshot is created whenever the cluster membership changes
- * so that clients don't need to hold a lock while examining a snapshot.
- */
-public class MembershipSnapshot {
-
-  // The latest instance of the MembershipSnapshot.
-  private static AtomicReference<MembershipSnapshot> cluster_ =
-      new AtomicReference<MembershipSnapshot>(new MembershipSnapshot());
-
-  // The set of hosts that are members of the cluster given by hostname.
-  private final Set<String> hostnames_;
-
-  // The set of hosts that are members of the cluster given by IP address.
-  private final Set<String> ipAddresses_;
-
-  // The number of nodes of the cluster.  Normally, this will be equal to
-  // hostnames_.size(), except in the test minicluster where there are multiple
-  // impalad's running on a single host.
-  private final int numNodes_;
-
-  // Used only to construct the initial MembershipSnapshot.  Before we get the first
-  // snapshot, assume one node (the localhost) to mimic Scheduler.
-  private MembershipSnapshot() {
-    hostnames_ = Sets.newHashSet();
-    ipAddresses_ = Sets.newHashSet();
-    numNodes_ = 1;
-  }
-
-  // Construct a new snapshot based on the TUpdateMembershipRequest.
-  private MembershipSnapshot(TUpdateMembershipRequest request) {
-    hostnames_ = request.getHostnames();
-    ipAddresses_ = request.getIp_addresses();
-    numNodes_ = request.getNum_nodes();
-  }
-
-  // Determine whether a host, given either by IP address or hostname, is a member of
-  // this snapshot.  Returns true if it is, false otherwise.
-  public boolean contains(TNetworkAddress address) {
-    String host = address.getHostname();
-    return ipAddresses_.contains(host) || hostnames_.contains(host);
-  }
-
-  // The number of nodes in this snapshot.
-  public int numNodes() { return numNodes_; }
-
-  // Atomically update the singleton snapshot instance.  After the update completes,
-  // all calls to getCluster() will return the new snapshot.
-  public static void update(TUpdateMembershipRequest request) {
-    cluster_.set(new MembershipSnapshot(request));
-  }
-
-  // Return the current singleton snapshot instance.
-  public static MembershipSnapshot getCluster() { return cluster_.get(); }
-
-}

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
index b671a1e..0c51036 100644
--- a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
+++ b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
@@ -70,8 +70,8 @@ import org.apache.impala.thrift.TScanRangeSpec;
 import org.apache.impala.thrift.TTableDescriptor;
 import org.apache.impala.thrift.TTableSink;
 import org.apache.impala.thrift.TTupleDescriptor;
-import org.apache.impala.thrift.TUpdateMembershipRequest;
-import org.apache.impala.util.MembershipSnapshot;
+import org.apache.impala.thrift.TUpdateExecutorMembershipRequest;
+import org.apache.impala.util.ExecutorMembershipSnapshot;
 import org.apache.kudu.client.KuduClient;
 import org.apache.kudu.client.KuduScanToken;
 import org.junit.AfterClass;
@@ -104,11 +104,11 @@ public class PlannerTestBase extends FrontendTestBase {
   @BeforeClass
   public static void setUp() throws Exception {
     // Mimic the 3 node test mini-cluster.
-    TUpdateMembershipRequest updateReq = new TUpdateMembershipRequest();
+    TUpdateExecutorMembershipRequest updateReq = new TUpdateExecutorMembershipRequest();
     updateReq.setIp_addresses(Sets.newHashSet("127.0.0.1"));
     updateReq.setHostnames(Sets.newHashSet("localhost"));
-    updateReq.setNum_nodes(3);
-    MembershipSnapshot.update(updateReq);
+    updateReq.setNum_executors(3);
+    ExecutorMembershipSnapshot.update(updateReq);
 
     if (RuntimeEnv.INSTANCE.isKuduSupported()) {
       kuduClient_ = new KuduClient.KuduClientBuilder("127.0.0.1:7051").build();

http://git-wip-us.apache.org/repos/asf/impala/blob/880011fa/tests/custom_cluster/test_coordinators.py
----------------------------------------------------------------------
diff --git a/tests/custom_cluster/test_coordinators.py b/tests/custom_cluster/test_coordinators.py
index 4ec3317..b49e328 100644
--- a/tests/custom_cluster/test_coordinators.py
+++ b/tests/custom_cluster/test_coordinators.py
@@ -245,3 +245,39 @@ class TestCoordinators(CustomClusterTestSuite):
       if client is not None:
         client.close()
       self._stop_impala_cluster()
+
+  @pytest.mark.execute_serially
+  def test_exclusive_coordinator_plan(self):
+    """Checks that a distributed plan does not assign scan fragments to a coordinator
+    only node. """
+
+    self._start_impala_cluster([], num_coordinators=1, cluster_size=3,
+        use_exclusive_coordinators=True)
+
+    assert len(self.cluster.impalads) == 3
+
+    coordinator = self.cluster.impalads[0]
+    worker1 = self.cluster.impalads[1]
+    worker2 = self.cluster.impalads[2]
+
+    client = None
+    try:
+      client = coordinator.service.create_beeswax_client()
+      assert client is not None
+      self.client = client
+
+      client.execute("SET EXPLAIN_LEVEL=2")
+
+      # Ensure that the plan generated always uses only the executor nodes for scanning
+      # Multi-partition table
+      result = client.execute("explain select count(*) from functional.alltypes "
+              "where id NOT IN (0,1,2) and string_col IN ('aaaa', 'bbbb', 'cccc', NULL) "
+              "and mod(int_col,50) IN (0,1) and id IN (int_col);").data
+      assert 'F00:PLAN FRAGMENT [RANDOM] hosts=2 instances=2' in result
+      # Single partition table
+      result = client.execute("explain select * from tpch.lineitem "
+              "union all select * from tpch.lineitem").data
+      assert 'F02:PLAN FRAGMENT [RANDOM] hosts=2 instances=2' in result
+    finally:
+      assert client is not None
+      self._stop_impala_cluster()


[2/6] impala git commit: IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell

Posted by ar...@apache.org.
IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell

The change handles the exception thrown by shlex while parsing a
malformed query.

This patch was tested by adding both commandline and interactive
shell tests.

Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d
Reviewed-on: http://gerrit.cloudera.org:8080/10876
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


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

Branch: refs/heads/master
Commit: 28162117ad462dfe5fd608d4c09ba02ad213285b
Parents: 54b3c60
Author: poojanilangekar <po...@cloudera.com>
Authored: Thu Jul 5 15:52:23 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Sat Jul 7 02:58:40 2018 +0000

----------------------------------------------------------------------
 shell/impala_shell.py                 | 15 +++++++++------
 tests/shell/test_shell_commandline.py |  9 +++++++++
 tests/shell/test_shell_interactive.py |  7 +++++++
 3 files changed, 25 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/28162117/shell/impala_shell.py
----------------------------------------------------------------------
diff --git a/shell/impala_shell.py b/shell/impala_shell.py
index 480c908..aea8350 100755
--- a/shell/impala_shell.py
+++ b/shell/impala_shell.py
@@ -1141,12 +1141,15 @@ class ImpalaShell(object, cmd.Cmd):
     lexer = shlex.shlex(sqlparse.format(query.query.lstrip(), strip_comments=True)
                         .encode('utf-8'), posix=True)
     lexer.escapedquotes += "'"
-    # Because the WITH clause may precede DML or SELECT queries,
-    # just checking the first token is insufficient.
-    is_dml = False
-    tokens = list(lexer)
-    if filter(self.DML_REGEX.match, tokens): is_dml = True
-    return self._execute_stmt(query, is_dml=is_dml, print_web_link=True)
+    try:
+      # Because the WITH clause may precede DML or SELECT queries,
+      # just checking the first token is insufficient.
+      is_dml = False
+      tokens = list(lexer)
+      if filter(self.DML_REGEX.match, tokens): is_dml = True
+      return self._execute_stmt(query, is_dml=is_dml, print_web_link=True)
+    except ValueError as e:
+      return self._execute_stmt(query, print_web_link=True)
 
   def do_use(self, args):
     """Executes a USE... query"""

http://git-wip-us.apache.org/repos/asf/impala/blob/28162117/tests/shell/test_shell_commandline.py
----------------------------------------------------------------------
diff --git a/tests/shell/test_shell_commandline.py b/tests/shell/test_shell_commandline.py
index 29df602..93387fa 100644
--- a/tests/shell/test_shell_commandline.py
+++ b/tests/shell/test_shell_commandline.py
@@ -650,3 +650,12 @@ class TestImpalaShell(ImpalaTestSuite):
       self._validate_expected_socket_connected(args2, s)
     finally:
       s.close()
+
+  def test_malformed_query(self):
+    """Test that malformed queries are handled by the commandline shell"""
+    args = " -q \"with foo as (select bar from temp where temp.a='\""
+    result = run_impala_shell_cmd(args, expect_success=False)
+    assert "Encountered: EOF" in result.stderr
+    args = "-q \"with v as (select 1) \;\""
+    result = run_impala_shell_cmd(args, expect_success=False)
+    assert "Encountered: Unexpected character" in result.stderr

http://git-wip-us.apache.org/repos/asf/impala/blob/28162117/tests/shell/test_shell_interactive.py
----------------------------------------------------------------------
diff --git a/tests/shell/test_shell_interactive.py b/tests/shell/test_shell_interactive.py
index eac9d27..bf4923d 100755
--- a/tests/shell/test_shell_interactive.py
+++ b/tests/shell/test_shell_interactive.py
@@ -654,6 +654,13 @@ class TestImpalaShellInteractive(object):
     assert (None, 'select 1') == \
         ImpalaShellClass.strip_leading_comment('select 1')
 
+  def test_malformed_query(self):
+    """Test the handling of malformed query without closing quotation"""
+    shell = ImpalaShell()
+    query = "with v as (select 1) \nselect foo('\\\\'), ('bar \n;"
+    shell.send_cmd(query)
+    result = shell.get_result()
+    assert "ERROR: AnalysisException: Unmatched string literal" in result.stderr
 
 def run_impala_shell_interactive(input_lines, shell_args=None):
   """Runs a command in the Impala shell interactively."""