Repository: incubator-impala Updated Branches: refs/heads/master f407288d7 -> c1da1409b
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata() Previously all code paths using getDbsMetadata() sufferred unnecessary privilege checks: 1. Impala checked privilege of all databases, tables before applying user provided JDBC pattern filters. 2. Impala passed a null pattern to getDbsMetadata() when user did not provide one. However, null pattern is treated as "%", which matches everything thereby causing unnecessary privilege checks for catalog objects that are not in the result set. This patch creates PatternMatcher early so that user specified null pattern is respected when calling getDbsMetadata(). Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Reviewed-on: http://gerrit.cloudera.org:8080/3371 Reviewed-by: Huaisi Xu <[email protected]> Tested-by: Huaisi Xu <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/c1da1409 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/c1da1409 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/c1da1409 Branch: refs/heads/master Commit: c1da1409ba8c915b7e59e74d924a9c823f48ae2d Parents: f407288 Author: Huaisi Xu <[email protected]> Authored: Mon Jun 13 18:57:35 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Thu Jul 7 10:41:29 2016 -0700 ---------------------------------------------------------------------- .../com/cloudera/impala/catalog/Catalog.java | 88 +++++-------- .../impala/catalog/CatalogServiceCatalog.java | 2 +- .../java/com/cloudera/impala/catalog/Db.java | 11 +- .../com/cloudera/impala/service/Frontend.java | 35 +++--- .../com/cloudera/impala/service/JniCatalog.java | 6 +- .../cloudera/impala/service/JniFrontend.java | 23 +++- .../com/cloudera/impala/service/MetadataOp.java | 75 +++++------ .../cloudera/impala/util/PatternMatcher.java | 48 ++++--- .../impala/analysis/AuthorizationTest.java | 126 ++++++++++++++++++- .../impala/testutil/BlockIdGenerator.java | 3 +- .../impala/testutil/ImpaladTestCatalog.java | 3 +- .../queries/QueryTest/show.test | 52 ++++++++ 12 files changed, 329 insertions(+), 143 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java b/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java index 991ead9..c0d1bb1 100644 --- a/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java +++ b/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java @@ -129,13 +129,10 @@ public abstract class Catalog { } /** - * Returns databases that match dbPattern. See filterStringsByPattern for details of - * the pattern match semantics. - * - * dbPattern may be null (and thus matches everything). + * Returns all databases that match 'matcher'. */ - public List<Db> getDbs(String dbPattern) { - return filterCatalogObjectsByPattern(dbCache_.get().values(), dbPattern); + public List<Db> getDbs(PatternMatcher matcher) { + return filterCatalogObjectsByPattern(dbCache_.get().values(), matcher); } /** @@ -163,22 +160,20 @@ public abstract class Catalog { } /** - * Returns a list of tables in the supplied database that match - * tablePattern. See filterStringsByPattern for details of the pattern match semantics. + * Returns all tables in 'dbName' that match 'matcher'. * - * dbName must not be null, but tablePattern may be null (and thus matches - * everything). + * dbName must not be null. * * Table names are returned unqualified. */ - public List<String> getTableNames(String dbName, String tablePattern) + public List<String> getTableNames(String dbName, PatternMatcher matcher) throws DatabaseNotFoundException { Preconditions.checkNotNull(dbName); Db db = getDb(dbName); if (db == null) { throw new DatabaseNotFoundException("Database '" + dbName + "' not found"); } - return filterStringsByPattern(db.getAllTableNames(), tablePattern); + return filterStringsByPattern(db.getAllTableNames(), matcher); } /** @@ -224,23 +219,22 @@ public abstract class Catalog { } /** - * Returns a list of data sources names that match pattern. See filterStringsByPattern - * for details of the pattern match semantics. + * Returns a list of data sources names that match pattern. + * + * @see PatternMatcher#matches(String) for details of the pattern match semantics. * * pattern may be null (and thus matches everything). */ public List<String> getDataSourceNames(String pattern) { - return filterStringsByPattern(dataSources_.keySet(), pattern); + return filterStringsByPattern(dataSources_.keySet(), + PatternMatcher.createHivePatternMatcher(pattern)); } /** - * Returns a list of data sources that match pattern. See filterStringsByPattern - * for details of the pattern match semantics. - * - * pattern may be null (and thus matches everything). + * Returns all DataSources that match 'matcher'. */ - public List<DataSource> getDataSources(String pattern) { - return filterCatalogObjectsByPattern(dataSources_.getValues(), pattern); + public List<DataSource> getDataSources(PatternMatcher matcher) { + return filterCatalogObjectsByPattern(dataSources_.getValues(), matcher); } /** @@ -326,26 +320,16 @@ public abstract class Catalog { public MetaStoreClient getMetaStoreClient() { return metaStoreClientPool_.getClient(); } /** - * Implement Hive's pattern-matching semantics for SHOW statements. The only - * metacharacters are '*' which matches any string of characters, and '|' - * which denotes choice. Doing the work here saves loading tables or - * databases from the metastore (which Hive would do if we passed the call - * through to the metastore client). - * - * If matchPattern is null, all strings are considered to match. If it is the - * empty string, no strings match. + * Return all members of 'candidates' that match 'matcher'. + * The results are sorted in String.CASE_INSENSITIVE_ORDER. + * matcher must not be null. */ private List<String> filterStringsByPattern(Iterable<String> candidates, - String matchPattern) { - List<String> filtered; - if (matchPattern == null) { - filtered = Lists.newArrayList(candidates); - } else { - PatternMatcher matcher = PatternMatcher.createHivePatternMatcher(matchPattern); - filtered = Lists.newArrayList(); - for (String candidate: candidates) { - if (matcher.matches(candidate)) filtered.add(candidate); - } + PatternMatcher matcher) { + Preconditions.checkNotNull(matcher); + List<String> filtered = Lists.newArrayList(); + for (String candidate: candidates) { + if (matcher.matches(candidate)) filtered.add(candidate); } Collections.sort(filtered, String.CASE_INSENSITIVE_ORDER); return filtered; @@ -361,26 +345,16 @@ public abstract class Catalog { private static final CatalogObjectOrder CATALOG_OBJECT_ORDER = new CatalogObjectOrder(); /** - * Implement Hive's pattern-matching semantics for SHOW statements. The only - * metacharacters are '*' which matches any string of characters, and '|' - * which denotes choice. Doing the work here saves loading tables or - * databases from the metastore (which Hive would do if we passed the call - * through to the metastore client). - * - * If matchPattern is null, all strings are considered to match. If it is the - * empty string, no strings match. + * Return all members of 'candidates' that match 'matcher'. + * The results are sorted in CATALOG_OBJECT_ORDER. + * matcher must not be null. */ private <T extends CatalogObject> List<T> filterCatalogObjectsByPattern( - Iterable<? extends T> candidates, String matchPattern) { - List<T> filtered; - if (matchPattern == null) { - filtered = Lists.newArrayList(candidates); - } else { - PatternMatcher matcher = PatternMatcher.createHivePatternMatcher(matchPattern); - filtered = Lists.newArrayList(); - for (T candidate: candidates) { - if (matcher.matches(candidate.getName())) filtered.add(candidate); - } + Iterable<? extends T> candidates, PatternMatcher matcher) { + Preconditions.checkNotNull(matcher); + List<T> filtered = Lists.newArrayList(); + for (T candidate: candidates) { + if (matcher.matches(candidate.getName())) filtered.add(candidate); } Collections.sort(filtered, CATALOG_OBJECT_ORDER); return filtered; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java b/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java index a55b0d3..f8ae864 100644 --- a/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java +++ b/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java @@ -259,7 +259,7 @@ public class CatalogServiceCatalog extends Catalog { resp.setMax_catalog_version(Catalog.INITIAL_CATALOG_VERSION); catalogLock_.readLock().lock(); try { - for (Db db: getDbs(null)) { + for (Db db: getDbs(PatternMatcher.MATCHER_MATCH_ALL)) { TCatalogObject catalogDb = new TCatalogObject(TCatalogObjectType.DATABASE, db.getCatalogVersion()); catalogDb.setDb(db.toThrift()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/catalog/Db.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/catalog/Db.java b/fe/src/main/java/com/cloudera/impala/catalog/Db.java index 114ac5e..9110080 100644 --- a/fe/src/main/java/com/cloudera/impala/catalog/Db.java +++ b/fe/src/main/java/com/cloudera/impala/catalog/Db.java @@ -428,17 +428,18 @@ public class Db implements CatalogObject { } /** - * Returns all functions that match 'fnPattern'. + * Returns all functions that match the pattern of 'matcher'. */ public List<Function> getFunctions(TFunctionCategory category, - PatternMatcher fnPattern) { + PatternMatcher matcher) { + Preconditions.checkNotNull(matcher); List<Function> result = Lists.newArrayList(); synchronized (functions_) { for (Map.Entry<String, List<Function>> fns: functions_.entrySet()) { - if (!fnPattern.matches(fns.getKey())) continue; + if (!matcher.matches(fns.getKey())) continue; for (Function fn: fns.getValue()) { - if (fn.userVisible() && - (category == null || Function.categoryMatch(fn, category))) { + if ((category == null || Function.categoryMatch(fn, category)) + && fn.userVisible()) { result.add(fn); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/service/Frontend.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/service/Frontend.java b/fe/src/main/java/com/cloudera/impala/service/Frontend.java index 9288c05..e8d360f 100644 --- a/fe/src/main/java/com/cloudera/impala/service/Frontend.java +++ b/fe/src/main/java/com/cloudera/impala/service/Frontend.java @@ -589,18 +589,18 @@ public class Frontend { } /** - * Returns all tables that match the specified database and pattern that are accessible - * to the given user. If pattern is null, matches all tables. If db is null, all - * databases are searched for matches. + * Returns all tables in database 'dbName' that match the pattern of 'matcher' and are + * accessible to 'user'. */ - public List<String> getTableNames(String dbName, String tablePattern, User user) - throws ImpalaException { - List<String> tblNames = impaladCatalog_.getTableNames(dbName, tablePattern); + public List<String> getTableNames(String dbName, PatternMatcher matcher, + User user) throws ImpalaException { + List<String> tblNames = impaladCatalog_.getTableNames(dbName, matcher); if (authzConfig_.isEnabled()) { Iterator<String> iter = tblNames.iterator(); while (iter.hasNext()) { + String tblName = iter.next(); PrivilegeRequest privilegeRequest = new PrivilegeRequestBuilder() - .any().onAnyColumn(dbName, iter.next()).toRequest(); + .any().onAnyColumn(dbName, tblName).toRequest(); if (!authzChecker_.get().hasAccess(user, privilegeRequest)) { iter.remove(); } @@ -610,16 +610,17 @@ public class Frontend { } /** - * Returns all columns of a table that match a pattern and are accessible to - * the given user. If pattern is null, it matches all columns. + * Returns a list of columns of a table using 'matcher' and are accessible + * to the given user. */ - public List<Column> getColumns(Table table, PatternMatcher columnPattern, + public List<Column> getColumns(Table table, PatternMatcher matcher, User user) throws InternalException { Preconditions.checkNotNull(table); + Preconditions.checkNotNull(matcher); List<Column> columns = Lists.newArrayList(); for (Column column: table.getColumnsInHiveOrder()) { String colName = column.getName(); - if (!columnPattern.matches(colName)) continue; + if (!matcher.matches(colName)) continue; if (authzConfig_.isEnabled()) { PrivilegeRequest privilegeRequest = new PrivilegeRequestBuilder() .any().onColumn(table.getTableName().getDb(), table.getTableName().getTbl(), @@ -632,11 +633,12 @@ public class Frontend { } /** - * Returns all databases that match the pattern and - * are accessible to the given user. If pattern is null, matches all dbs. + * Returns all databases in catalog cache that match the pattern of 'matcher' and are + * accessible to 'user'. */ - public List<Db> getDbs(String dbPattern, User user) throws InternalException { - List<Db> dbs = impaladCatalog_.getDbs(dbPattern); + public List<Db> getDbs(PatternMatcher matcher, User user) + throws InternalException { + List<Db> dbs = impaladCatalog_.getDbs(matcher); // If authorization is enabled, filter out the databases the user does not // have permissions on. if (authzConfig_.isEnabled()) { @@ -668,7 +670,8 @@ public class Frontend { * matches all data sources. */ public List<DataSource> getDataSrcs(String pattern) { - return impaladCatalog_.getDataSources(pattern); + return impaladCatalog_.getDataSources( + PatternMatcher.createHivePatternMatcher(pattern)); } /** http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java b/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java index 4a4d529..ff70c9e 100644 --- a/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java +++ b/fe/src/main/java/com/cloudera/impala/service/JniCatalog.java @@ -52,6 +52,7 @@ import com.cloudera.impala.thrift.TSentryAdminCheckRequest; import com.cloudera.impala.thrift.TUniqueId; import com.cloudera.impala.thrift.TUpdateCatalogRequest; import com.cloudera.impala.util.GlogAppender; +import com.cloudera.impala.util.PatternMatcher; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Lists; @@ -156,7 +157,7 @@ public class JniCatalog { TException { TGetDbsParams params = new TGetDbsParams(); JniUtil.deserializeThrift(protocolFactory_, params, thriftGetTablesParams); - List<Db> dbs = catalog_.getDbs(null); + List<Db> dbs = catalog_.getDbs(PatternMatcher.MATCHER_MATCH_ALL); TGetDbsResult result = new TGetDbsResult(); List<TDatabase> tDbs = Lists.newArrayListWithCapacity(dbs.size()); for (Db db: dbs) tDbs.add(db.toThrift()); @@ -174,7 +175,8 @@ public class JniCatalog { TException { TGetTablesParams params = new TGetTablesParams(); JniUtil.deserializeThrift(protocolFactory_, params, thriftGetTablesParams); - List<String> tables = catalog_.getTableNames(params.db, params.pattern); + List<String> tables = catalog_.getTableNames(params.db, + PatternMatcher.createHivePatternMatcher(params.pattern)); TGetTablesResult result = new TGetTablesResult(); result.setTables(tables); TSerializer serializer = new TSerializer(protocolFactory_); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java b/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java index 2256900..ccba28d 100644 --- a/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java +++ b/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java @@ -85,6 +85,7 @@ import com.cloudera.impala.thrift.TUniqueId; import com.cloudera.impala.thrift.TUpdateCatalogCacheRequest; import com.cloudera.impala.thrift.TUpdateMembershipRequest; import com.cloudera.impala.util.GlogAppender; +import com.cloudera.impala.util.PatternMatcher; import com.cloudera.impala.util.TSessionStateUtil; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; @@ -224,7 +225,14 @@ public class JniFrontend { } /** - * Returns a list of table names matching an optional pattern. + * Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']", and + * return a list of table names matching an optional pattern. + * The only metacharacters are '*' which matches any string of characters, and '|' + * which denotes choice. Doing the work here saves loading tables or databases from the + * metastore (which Hive would do if we passed the call through to the metastore + * client). If the pattern is null, all strings are considered to match. If it is an + * empty string, no strings match. + * * The argument is a serialized TGetTablesParams object. * The return type is a serialised TGetTablesResult object. * @see Frontend#getTableNames @@ -238,7 +246,8 @@ public class JniFrontend { ImpalaInternalAdminUser.getInstance(); Preconditions.checkState(!params.isSetSession() || user != null ); - List<String> tables = frontend_.getTableNames(params.db, params.pattern, user); + List<String> tables = frontend_.getTableNames(params.db, + PatternMatcher.createHivePatternMatcher(params.pattern), user); TGetTablesResult result = new TGetTablesResult(); result.setTables(tables); @@ -271,10 +280,13 @@ public class JniFrontend { } /** - * Returns a list of databases matching an optional pattern. + * Implement Hive's pattern-matching semantics for "SHOW DATABASES [[LIKE] 'pattern']", + * and return a list of databases matching an optional pattern. + * @see JniFrontend#getTableNames(byte[]) for more detail. + * * The argument is a serialized TGetDbParams object. * The return type is a serialised TGetDbResult object. - * @see Frontend#getDbParams + * @see Frontend#getDbs */ public byte[] getDbs(byte[] thriftGetTablesParams) throws ImpalaException { TGetDbsParams params = new TGetDbsParams(); @@ -283,7 +295,8 @@ public class JniFrontend { User user = params.isSetSession() ? new User(TSessionStateUtil.getEffectiveUser(params.getSession())) : ImpalaInternalAdminUser.getInstance(); - List<Db> dbs = frontend_.getDbs(params.pattern, user); + List<Db> dbs = frontend_.getDbs( + PatternMatcher.createHivePatternMatcher(params.pattern), user); TGetDbsResult result = new TGetDbsResult(); List<TDatabase> tDbs = Lists.newArrayListWithCapacity(dbs.size()); for (Db db: dbs) tDbs.add(db.toThrift()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java b/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java index db5e96b..c07a566 100644 --- a/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java +++ b/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java @@ -223,27 +223,28 @@ public class MetadataOp { } /** - * Returns the list of schemas, tables, columns and user functions that satisfy the - * search pattern. catalogName, schemaName, tableName, columnName and functionName - * are JDBC search patterns. + * Returns the list of schemas, tables, columns and user functions that match the + * corresponding matchers. * - * The return value DbsTablesColumns.dbs contains the list of databases that satisfy - * the "schemaName" search pattern. - * DbsTablesColumns.tableNames[i] contains the list of tables inside dbs[i] that satisfy - * the "tableName" search pattern. - * DbsTablesColumns.columns[i][j] contains the list of columns of table[j] in dbs[i] - * that satisfy the search condition "columnName". - * DbsTablesColumns.functions[i] contains the list of functions inside dbs[i] that - * satisfy the "functionName" search pattern. + * The return value 'result.dbs' contains the list of databases that match + * 'schemaPatternMatcher'. + * 'result.tableNames[i]' contains the list of tables inside dbs[i] that match + * 'tablePatternMatcher'. + * 'result.columns[i][j]' contains the list of columns of table[j] in dbs[i] + * that match 'columnPatternMatcher'. + * result.functions[i] contains the list of functions inside dbs[i] that + * match 'fnPatternMatcher'. * - * If functionName is not null, then only function metadata will be returned. - * If tableName is null, then DbsTablesColumns.tableNames and DbsTablesColumns.columns - * will not be populated. - * If columns is null, then DbsTablesColumns.columns will not be populated. + * If 'fnPatternMatcher' is not PatternMatcher.MATCHER_MATCH_NONE, then only function + * metadata will be returned. + * If 'tablePatternMatcher' is PatternMatcher.MATCHER_MATCH_NONE, then + * 'result.tableNames' and 'result.columns' will not be populated. + * If columns is null, then 'result.columns' will not be populated. */ private static DbsMetadata getDbsMetadata(Frontend fe, String catalogName, - String schemaName, String tableName, String columnName, String functionName, - User user) throws ImpalaException { + PatternMatcher schemaPatternMatcher, PatternMatcher tablePatternMatcher, + PatternMatcher columnPatternMatcher, PatternMatcher fnPatternMatcher, User user) + throws ImpalaException { DbsMetadata result = new DbsMetadata(); // Hive does not have a catalog concept. Returns nothing if the request specifies an @@ -252,25 +253,17 @@ public class MetadataOp { return result; } - // Creates the schema, table, column and function search patterns - PatternMatcher schemaPattern = PatternMatcher.createJdbcPatternMatcher(schemaName); - PatternMatcher tablePattern = PatternMatcher.createJdbcPatternMatcher(tableName); - PatternMatcher columnPattern = PatternMatcher.createJdbcPatternMatcher(columnName); - PatternMatcher fnPattern = PatternMatcher.createJdbcPatternMatcher(functionName); - ImpaladCatalog catalog = fe.getCatalog(); - for (Db db: fe.getDbs(null, user)) { - if (!schemaPattern.matches(db.getName())) continue; - if (functionName != null) { + for (Db db: fe.getDbs(schemaPatternMatcher, user)) { + if (fnPatternMatcher != PatternMatcher.MATCHER_MATCH_NONE) { // Get function metadata - List<Function> fns = db.getFunctions(null, fnPattern); + List<Function> fns = db.getFunctions(null, fnPatternMatcher); result.functions.add(fns); } else { // Get table metadata List<String> tableList = Lists.newArrayList(); List<List<Column>> tablesColumnsList = Lists.newArrayList(); - for (String tabName: fe.getTableNames(db.getName(), "*", user)) { - if (!tablePattern.matches(tabName)) continue; + for (String tabName: fe.getTableNames(db.getName(), tablePatternMatcher, user)) { tableList.add(tabName); Table table = null; try { @@ -286,7 +279,7 @@ public class MetadataOp { if (!table.isLoaded()) { result.missingTbls.add(new TableName(db.getName(), tabName)); } else { - columns.addAll(fe.getColumns(table, columnPattern, user)); + columns.addAll(fe.getColumns(table, columnPatternMatcher, user)); } tablesColumnsList.add(columns); } @@ -326,9 +319,12 @@ public class MetadataOp { // Get the list of schemas, tables, and columns that satisfy the search conditions. DbsMetadata dbsMetadata = null; + PatternMatcher schemaMatcher = PatternMatcher.createJdbcPatternMatcher(schemaName); + PatternMatcher tableMatcher = PatternMatcher.createJdbcPatternMatcher(tableName); + PatternMatcher columnMatcher = PatternMatcher.createJdbcPatternMatcher(columnName); while (dbsMetadata == null || !dbsMetadata.missingTbls.isEmpty()) { - dbsMetadata = getDbsMetadata(fe, catalogName, - schemaName, tableName, columnName, null, user); + dbsMetadata = getDbsMetadata(fe, catalogName, schemaMatcher, tableMatcher, + columnMatcher, PatternMatcher.MATCHER_MATCH_NONE, user); if (!fe.requestTblLoadAndWait(dbsMetadata.missingTbls)) { LOG.info("Timed out waiting for missing tables. Load request will be retried."); } @@ -413,7 +409,10 @@ public class MetadataOp { // Get the list of schemas that satisfy the search condition. DbsMetadata dbsMetadata = getDbsMetadata(fe, catalogName, - schemaName, null, null, null, user); + PatternMatcher.createJdbcPatternMatcher(schemaName), + PatternMatcher.MATCHER_MATCH_NONE, + PatternMatcher.MATCHER_MATCH_NONE, + PatternMatcher.MATCHER_MATCH_NONE, user); for (int i = 0; i < dbsMetadata.dbs.size(); ++i) { String dbName = dbsMetadata.dbs.get(i); @@ -457,7 +456,10 @@ public class MetadataOp { // Get the list of schemas, tables that satisfy the search conditions. DbsMetadata dbsMetadata = getDbsMetadata(fe, catalogName, - schemaName, tableName, null, null, user); + PatternMatcher.createJdbcPatternMatcher(schemaName), + PatternMatcher.createJdbcPatternMatcher(tableName), + PatternMatcher.MATCHER_MATCH_NONE, + PatternMatcher.MATCHER_MATCH_NONE, user); for (int i = 0; i < dbsMetadata.dbs.size(); ++i) { String dbName = dbsMetadata.dbs.get(i); @@ -529,7 +531,10 @@ public class MetadataOp { } DbsMetadata dbsMetadata = getDbsMetadata(fe, catalogName, - schemaName, null, null, functionName, user); + PatternMatcher.createJdbcPatternMatcher(schemaName), + PatternMatcher.MATCHER_MATCH_NONE, + PatternMatcher.MATCHER_MATCH_NONE, + PatternMatcher.createJdbcPatternMatcher(functionName), user); for (List<Function> fns: dbsMetadata.functions) { for (Function fn: fns) { result.rows.add(createFunctionResultRow(fn)); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java b/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java index 18f282b..f4aaadc 100644 --- a/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java +++ b/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java @@ -15,6 +15,7 @@ package com.cloudera.impala.util; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -31,7 +32,8 @@ public class PatternMatcher { // any of the patterns. private List<Pattern> patterns_; - // Returns true if the candidate matches. + // Returns true if patterns_ is null or the candidate matches. + // Returns false if patterns_ is empty or the candidate mismatches. public boolean matches(String candidate) { if (patterns_ == null) return true; if (patterns_.isEmpty()) return false; @@ -41,6 +43,21 @@ public class PatternMatcher { return false; } + // Immutable pattern matcher that matches all + private final static class MatchAllPatternMatcher extends PatternMatcher { + MatchAllPatternMatcher() {} + public boolean matches(String candidate) { return true; } + } + + // Immutable pattern matcher that matches none + private final static class MatchNonePatternMatcher extends PatternMatcher { + MatchNonePatternMatcher() {} + public boolean matches(String candidate) { return false; } + } + + public static final PatternMatcher MATCHER_MATCH_ALL = new MatchAllPatternMatcher(); + public static final PatternMatcher MATCHER_MATCH_NONE = new MatchNonePatternMatcher(); + /** * Creates a pattern matcher for hive patterns. * The only metacharacters are '*' which matches any string of characters, and '|' @@ -49,18 +66,18 @@ public class PatternMatcher { * empty string, no strings match. */ public static PatternMatcher createHivePatternMatcher(String hivePattern) { + if (hivePattern == null) return MATCHER_MATCH_ALL; + if (hivePattern.isEmpty()) return MATCHER_MATCH_NONE; PatternMatcher result = new PatternMatcher(); - if (hivePattern != null) { - result.patterns_ = Lists.newArrayList(); - // Hive ignores pretty much all metacharacters, so we have to escape them. - final String metaCharacters = "+?.^()]\\/{}"; - final Pattern regex = Pattern.compile("([" + Pattern.quote(metaCharacters) + "])"); + result.patterns_ = Lists.newArrayList(); + // Hive ignores pretty much all metacharacters, so we have to escape them. + final String metaCharacters = "+?.^()]\\/{}"; + final Pattern regex = Pattern.compile("([" + Pattern.quote(metaCharacters) + "])"); - for (String pattern: Arrays.asList(hivePattern.split("\\|"))) { - Matcher matcher = regex.matcher(pattern); - pattern = matcher.replaceAll("\\\\$1").replace("*", ".*"); - result.patterns_.add(Pattern.compile(pattern)); - } + for (String pattern: Arrays.asList(hivePattern.split("\\|"))) { + Matcher matcher = regex.matcher(pattern); + pattern = matcher.replaceAll("\\\\$1").replace("*", ".*"); + result.patterns_.add(Pattern.compile(pattern)); } return result; } @@ -69,12 +86,11 @@ public class PatternMatcher { * Creates a matcher object for JDBC match strings. */ public static PatternMatcher createJdbcPatternMatcher(String pattern) { - String wildcardPattern = ".*"; - String workPattern = pattern; - if (workPattern == null || pattern.isEmpty()) { - workPattern = "%"; + if (pattern == null || pattern.isEmpty()) { + return MATCHER_MATCH_ALL; } - String result = workPattern + String wildcardPattern = ".*"; + String result = pattern .replaceAll("([^\\\\])%", "$1" + wildcardPattern) .replaceAll("\\\\%", "%") .replaceAll("^%", wildcardPattern) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/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 b82d5d4..99a8d17 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.UUID; @@ -68,6 +69,7 @@ import com.cloudera.impala.thrift.TPrivilegeScope; import com.cloudera.impala.thrift.TQueryCtx; import com.cloudera.impala.thrift.TResultSet; import com.cloudera.impala.thrift.TSessionState; +import com.cloudera.impala.util.PatternMatcher; import com.cloudera.impala.util.SentryPolicyService; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; @@ -1536,10 +1538,26 @@ public class AuthorizationTest { List<String> expectedDbs = Lists.newArrayList("default", "functional", "functional_parquet", "functional_seq_snap", "tpcds", "tpch"); - List<Db> dbs = fe_.getDbs("*", USER); + List<Db> dbs = fe_.getDbs(PatternMatcher.createHivePatternMatcher("*"), USER); assertEquals(expectedDbs, extractDbNames(dbs)); - dbs = fe_.getDbs(null, USER); + dbs = fe_.getDbs(PatternMatcher.MATCHER_MATCH_ALL, USER); + assertEquals(expectedDbs, extractDbNames(dbs)); + + dbs = fe_.getDbs(PatternMatcher.createHivePatternMatcher(null), USER); + assertEquals(expectedDbs, extractDbNames(dbs)); + + dbs = fe_.getDbs(PatternMatcher.MATCHER_MATCH_NONE, USER); + assertEquals(Collections.emptyList(), extractDbNames(dbs)); + + dbs = fe_.getDbs(PatternMatcher.createHivePatternMatcher(""), USER); + assertEquals(Collections.emptyList(), extractDbNames(dbs)); + + dbs = fe_.getDbs(PatternMatcher.createHivePatternMatcher("functional_rc"), USER); + assertEquals(Collections.emptyList(), extractDbNames(dbs)); + + dbs = fe_.getDbs(PatternMatcher.createHivePatternMatcher("tp*|de*"), USER); + expectedDbs = Lists.newArrayList("default", "tpcds", "tpch"); assertEquals(expectedDbs, extractDbNames(dbs)); } @@ -1551,11 +1569,31 @@ public class AuthorizationTest { @Test public void TestShowTableResultsFiltered() throws ImpalaException { - List<String> tables = fe_.getTableNames("functional", "*", USER); + List<String> tables = fe_.getTableNames("functional", + PatternMatcher.createHivePatternMatcher("*"), USER); + Assert.assertEquals(FUNCTIONAL_VISIBLE_TABLES, tables); + + tables = fe_.getTableNames("functional", + PatternMatcher.MATCHER_MATCH_ALL, USER); Assert.assertEquals(FUNCTIONAL_VISIBLE_TABLES, tables); - tables = fe_.getTableNames("functional", null, USER); + tables = fe_.getTableNames("functional", + PatternMatcher.createHivePatternMatcher(null), USER); Assert.assertEquals(FUNCTIONAL_VISIBLE_TABLES, tables); + + tables = fe_.getTableNames("functional", + PatternMatcher.MATCHER_MATCH_NONE, USER); + Assert.assertEquals(Collections.emptyList(), tables); + + tables = fe_.getTableNames("functional", + PatternMatcher.createHivePatternMatcher(""), USER); + Assert.assertEquals(Collections.emptyList(), tables); + + tables = fe_.getTableNames("functional", + PatternMatcher.createHivePatternMatcher("alltypes*|view_view"), USER); + List<String> expectedTables = Lists.newArrayList( + "alltypes", "alltypesagg", "alltypessmall", "alltypestiny", "view_view"); + Assert.assertEquals(expectedTables, tables); } @Test @@ -1602,6 +1640,46 @@ public class AuthorizationTest { assertEquals(FUNCTIONAL_VISIBLE_TABLES.get(i), resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); } + // Pattern "" and null is the same as "%" to match all + req.get_tables_req.setTableName(""); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(FUNCTIONAL_VISIBLE_TABLES.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(FUNCTIONAL_VISIBLE_TABLES.get(i), + resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); + } + req.get_tables_req.setTableName(null); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(FUNCTIONAL_VISIBLE_TABLES.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(FUNCTIONAL_VISIBLE_TABLES.get(i), + resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); + } + // Pattern ".*" matches all and "." is a wildcard that matches any single character + req.get_tables_req.setTableName(".*"); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(FUNCTIONAL_VISIBLE_TABLES.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(FUNCTIONAL_VISIBLE_TABLES.get(i), + resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); + } + req.get_tables_req.setTableName("alltypesag."); + resp = fe_.execHiveServer2MetadataOp(req); + List<String> expectedTblNames = Lists.newArrayList("alltypesagg"); + assertEquals(expectedTblNames.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedTblNames.get(i), + resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); + } + // "_" is a wildcard for a single character + req.get_tables_req.setTableName("alltypesag_"); + resp = fe_.execHiveServer2MetadataOp(req); + expectedTblNames = Lists.newArrayList("alltypesagg"); + assertEquals(expectedTblNames.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedTblNames.get(i), + resp.rows.get(i).colVals.get(2).string_val.toLowerCase()); + } // Get all tables of tpcds req.get_tables_req.setSchemaName("tpcds"); @@ -1626,6 +1704,46 @@ public class AuthorizationTest { assertEquals(expectedDbs.get(i), resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); } + // Pattern "" and null is the same as "%" to match all + req.get_schemas_req.setSchemaName(""); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(expectedDbs.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedDbs.get(i), + resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); + } + req.get_schemas_req.setSchemaName(null); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(expectedDbs.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedDbs.get(i), + resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); + } + // Pattern ".*" matches all and "." is a wildcard that matches any single character + req.get_schemas_req.setSchemaName(".*"); + resp = fe_.execHiveServer2MetadataOp(req); + assertEquals(expectedDbs.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedDbs.get(i), + resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); + } + req.get_schemas_req.setSchemaName("defaul."); + resp = fe_.execHiveServer2MetadataOp(req); + expectedDbs = Lists.newArrayList("default"); + assertEquals(expectedDbs.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedDbs.get(i), + resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); + } + // "_" is a wildcard that matches any single character + req.get_schemas_req.setSchemaName("defaul_"); + resp = fe_.execHiveServer2MetadataOp(req); + expectedDbs = Lists.newArrayList("default"); + assertEquals(expectedDbs.size(), resp.rows.size()); + for (int i = 0; i < resp.rows.size(); ++i) { + assertEquals(expectedDbs.get(i), + resp.rows.get(i).colVals.get(0).string_val.toLowerCase()); + } } @Test http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java b/fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java index 11f7a0b..7c2df71 100644 --- a/fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java +++ b/fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java @@ -18,6 +18,7 @@ import com.cloudera.impala.catalog.HdfsPartition.FileDescriptor; import com.cloudera.impala.catalog.HdfsTable; import com.cloudera.impala.catalog.Table; import com.cloudera.impala.thrift.ImpalaInternalServiceConstants; +import com.cloudera.impala.util.PatternMatcher; /** * Utility to generate an output file with all the block ids for each table @@ -44,7 +45,7 @@ public class BlockIdGenerator { // Load all tables in the catalog Catalog catalog = CatalogServiceTestCatalog.create(); - for (Db database: catalog.getDbs(null)) { + for (Db database: catalog.getDbs(PatternMatcher.MATCHER_MATCH_ALL)) { for (String tableName: database.getAllTableNames()) { Table table = database.getTable(tableName); // Only do this for hdfs tables http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java b/fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java index 82a3fb6..d364fa2 100644 --- a/fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java +++ b/fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java @@ -21,6 +21,7 @@ import com.cloudera.impala.catalog.Db; import com.cloudera.impala.catalog.HdfsCachePool; import com.cloudera.impala.catalog.ImpaladCatalog; import com.cloudera.impala.catalog.Table; +import com.cloudera.impala.util.PatternMatcher; import com.google.common.base.Preconditions; /** @@ -43,7 +44,7 @@ public class ImpaladTestCatalog extends ImpaladCatalog { CatalogServiceCatalog catalogServerCatalog = CatalogServiceTestCatalog.createWithAuth(authzConfig.getSentryConfig()); // Bootstrap the catalog by adding all dbs, tables, and functions. - for (Db db: catalogServerCatalog.getDbs(null)) { + for (Db db: catalogServerCatalog.getDbs(PatternMatcher.MATCHER_MATCH_ALL)) { // Adding DB should include all tables/fns in that database. addDb(db); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c1da1409/testdata/workloads/functional-query/queries/QueryTest/show.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/show.test b/testdata/workloads/functional-query/queries/QueryTest/show.test index af54c74..1c8bb48 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/show.test +++ b/testdata/workloads/functional-query/queries/QueryTest/show.test @@ -154,6 +154,32 @@ show tables in functional like 'alltypesagg*' STRING ==== ---- QUERY +# Impala only considers '*' and '|' as meta-characters in SHOW statements +# See IMPALA-3744 +show tables in functional like 'alltypesag.' +---- RESULTS +---- TYPES +STRING +==== +---- QUERY +show tables in functional like 'alltypesag.*' +---- RESULTS +---- TYPES +STRING +==== +---- QUERY +show tables in functional like 'alltypesagg%' +---- RESULTS +---- TYPES +STRING +==== +---- QUERY +show tables in functional like 'alltypesag_' +---- RESULTS +---- TYPES +STRING +==== +---- QUERY # Coverage of syntax variations. show tables in functional 'alltypesagg' ---- RESULTS @@ -212,6 +238,32 @@ show databases like 'def*' STRING,STRING ==== ---- QUERY +# Impala only considers '*' and '|' as meta-characters in SHOW statements +# See IMPALA-3744 +show databases like 'defaul.*' +---- RESULTS +---- TYPES +STRING,STRING +==== +---- QUERY +show databases like 'defaul_' +---- RESULTS +---- TYPES +STRING,STRING +==== +---- QUERY +show databases like 'def%' +---- RESULTS +---- TYPES +STRING,STRING +==== +---- QUERY +show databases like 'defaul.' +---- RESULTS +---- TYPES +STRING,STRING +==== +---- QUERY show files in alltypesagg ---- LABELS Path,Size,Partition
