IMPALA-3269: Remove authz checks on default table location in CTAS queries Bug: In CreateTableAsSelectStmt.analyze(), we set the default location of table if the query doesn't explicitly set a table location. However this is an issue with CTAS with subqueries as they follow a two pass analysis with the second analyze() call failing with a authz error on the URI added in the first pass.
Fix: We needn't set the default location explicitly. Metastore automatically figures it out if it is not set. Change-Id: I0451586d4994cf9fc8c3dd47c8f3a513067cb2ea Reviewed-on: http://gerrit.cloudera.org:8080/2664 Reviewed-by: Bharath Vissapragada <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/49f9559f Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/49f9559f Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/49f9559f Branch: refs/heads/master Commit: 49f9559f204278e39dcd362786adcc7b2f950d9e Parents: 62bb864 Author: Bharath Vissapragada <[email protected]> Authored: Mon Mar 28 23:37:20 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Tue Apr 12 14:03:44 2016 -0700 ---------------------------------------------------------------------- .../impala/analysis/CreateTableAsSelectStmt.java | 7 ------- .../impala/analysis/CreateTableLikeFileStmt.java | 2 +- .../java/com/cloudera/impala/analysis/ToSqlUtils.java | 12 ++++++------ .../com/cloudera/impala/analysis/AuthorizationTest.java | 4 ++++ .../java/com/cloudera/impala/analysis/ToSqlTest.java | 4 ++-- 5 files changed, 13 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/49f9559f/fe/src/main/java/com/cloudera/impala/analysis/CreateTableAsSelectStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/CreateTableAsSelectStmt.java b/fe/src/main/java/com/cloudera/impala/analysis/CreateTableAsSelectStmt.java index 640f92c..15c8a37 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/CreateTableAsSelectStmt.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/CreateTableAsSelectStmt.java @@ -180,13 +180,6 @@ public class CreateTableAsSelectStmt extends StatementBase { // user specified a location for the table this will be a no-op. msTbl.getSd().setLocation(analyzer.getCatalog().getTablePath(msTbl).toString()); - // If the user didn't specify a table location for the CREATE statement, inject the - // location that was calculated in the getTablePath() call. Since this will be the - // target location for the INSERT statement, it is important the two match. - if (createStmt_.getLocation() == null) { - createStmt_.setLocation(new HdfsUri(msTbl.getSd().getLocation())); - } - // Create a "temp" table based off the given metastore.api.Table object. Normally, // the CatalogService assigns all table IDs, but in this case we need to assign the // "temp" table an ID locally. This table ID cannot conflict with any table in the http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/49f9559f/fe/src/main/java/com/cloudera/impala/analysis/CreateTableLikeFileStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/CreateTableLikeFileStmt.java b/fe/src/main/java/com/cloudera/impala/analysis/CreateTableLikeFileStmt.java index 54cfec0..7676d98 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/CreateTableLikeFileStmt.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/CreateTableLikeFileStmt.java @@ -350,7 +350,7 @@ public class CreateTableLikeFileStmt extends CreateTableStmt { getTbl() + " __LIKE_FILEFORMAT__ ", getComment(), colsSql, partitionColsSql, getTblProperties(), getSerdeProperties(), isExternal(), getIfNotExists(), getRowFormat(), HdfsFileFormat.fromThrift(getFileFormat()), - compression, null, getLocation().toString()); + compression, null, getLocation()); s = s.replace("__LIKE_FILEFORMAT__", "LIKE " + schemaFileFormat_ + " " + schemaLocation_.toString()); return s; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/49f9559f/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java b/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java index 0b95536..d597266 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/ToSqlUtils.java @@ -127,13 +127,12 @@ public class ToSqlUtils { for (ColumnDef col: stmt.getPartitionColumnDefs()) { partitionColsSql.add(col.toString()); } - String location = stmt.getLocation() == null ? null : stmt.getLocation().toString(); // TODO: Pass the correct compression, if applicable. return getCreateTableSql(stmt.getDb(), stmt.getTbl(), stmt.getComment(), colsSql, partitionColsSql, stmt.getTblProperties(), stmt.getSerdeProperties(), stmt.isExternal(), stmt.getIfNotExists(), stmt.getRowFormat(), HdfsFileFormat.fromThrift(stmt.getFileFormat()), HdfsCompression.NONE, null, - location); + stmt.getLocation()); } /** @@ -154,7 +153,7 @@ public class ToSqlUtils { innerStmt.getSerdeProperties(), innerStmt.isExternal(), innerStmt.getIfNotExists(), innerStmt.getRowFormat(), HdfsFileFormat.fromThrift(innerStmt.getFileFormat()), HdfsCompression.NONE, null, - innerStmt.getLocation().toString()); + innerStmt.getLocation()); return createTableSql + " AS " + stmt.getQueryStmt().toSql(); } @@ -199,9 +198,10 @@ public class ToSqlUtils { // Kudu tables cannot use the Hive DDL syntax for the storage handler storageHandlerClassName = null; } + HdfsUri tableLocation = location == null ? null : new HdfsUri(location); return getCreateTableSql(table.getDb().getName(), table.getName(), comment, colsSql, partitionColsSql, properties, serdeParameters, isExternal, false, rowFormat, - format, compression, storageHandlerClassName, location); + format, compression, storageHandlerClassName, tableLocation); } /** @@ -214,7 +214,7 @@ public class ToSqlUtils { Map<String, String> tblProperties, Map<String, String> serdeParameters, boolean isExternal, boolean ifNotExists, RowFormat rowFormat, HdfsFileFormat fileFormat, HdfsCompression compression, String storageHandlerClass, - String location) { + HdfsUri location) { Preconditions.checkNotNull(tableName); StringBuilder sb = new StringBuilder("CREATE "); if (isExternal) sb.append("EXTERNAL "); @@ -279,7 +279,7 @@ public class ToSqlUtils { } } if (location != null) { - sb.append("LOCATION '" + location + "'\n"); + sb.append("LOCATION '" + location.toString() + "'\n"); } if (tblProperties != null && !tblProperties.isEmpty()) { sb.append("TBLPROPERTIES " + propertyMapToSql(tblProperties)); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/49f9559f/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java b/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java index b456888..499fdf7 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java @@ -822,6 +822,10 @@ public class AuthorizationTest { "User '%s' does not have privileges to execute 'SELECT' on: " + "functional.alltypes"); + // CTAS with a subquery. + AuthzOk("create table tpch.new_table as select * from functional.alltypesagg " + + "where id < (select max(year) from functional.alltypesagg)"); + AuthzError("create table functional.tbl tblproperties('a'='b')" + " as select 1", "User '%s' does not have privileges to execute 'CREATE' on: " + http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/49f9559f/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java b/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java index 744a8d2..521b19f 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java @@ -304,8 +304,8 @@ public class ToSqlTest extends AnalyzerTest { "select double_col, int_col from functional.alltypes", "default", "CREATE TABLE default.p PARTITIONED BY ( int_col ) STORED AS " + - "TEXTFILE LOCATION 'hdfs://localhost:20500/test-warehouse/p' " + - "AS SELECT double_col, int_col FROM functional.alltypes", true); + "TEXTFILE AS SELECT double_col, int_col FROM functional.alltypes", + true); } @Test
