Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary ......................................................................
Patch Set 8: (25 comments) http://gerrit.cloudera.org:8080/#/c/3371/8//COMMIT_MSG Commit Message: PS8, Line 7: Check privileges when necessary Maybe "Remove unnecessary privilege checks when accessing catalog objects" PS8, Line 13: user only request database name The description is a bit misleading. In general, requesting a db from the catalog didn't have the same effect, no? Let's try to improve the description so that it is clear what problem this commit solves. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS8, Line 132: Returns databases using matcher "Returns all databases that match the pattern of 'matcher'." Similar comment for all the other functions. PS8, Line 134: matcher must not be null. You don't enforce that. Add a Preconditions.checkNotNull() above L137 or remove the comment if that condition is enforced in fileCatalogObjectsByPatterrn. Same for any other function that makes that assumption. PS8, Line 172: tablePatternMatcher Just rename to "matcher". PS8, Line 359: Filter candidates.getName() using matcher "Return all catalog objects from 'candidates' that match the pattern of 'matcher'. If 'matcher' doesn't have a pattern, all catalog objects in 'candidates' are returned. The results are sorted in CATALOG_OBJECT_ORDER. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Db.java File fe/src/main/java/com/cloudera/impala/catalog/Db.java: PS8, Line 17: * Please don't do that. We don't need everything from util. PS8, Line 427: Returns all functions using fnPatternMatcher "Returns all functions that match the pattern of 'matcher'. if 'matcher' is null, return an empty list." http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS8, Line 592: Match tables in database dbName with tablePatternMatcher and check user's privileges. "Returns all tables in database 'dbName' that match the pattern of 'matcher' and are accessible to the given user. Returns an empty list If 'matcher' is null." PS8, Line 595: Throw ImpalaException when dbName is null Where does this happen? Also when describing the conditions during which an exception is thrown you don't need to be that specific. You may remove this comment altogether, it's not that interesting. PS8, Line 598: tablePatternMatcher Let's rename all these variables to 'matcher' unless there are multiple instances that we need to differentiate. PS8, Line 624: List<Column> columns = Lists.newArrayList(); : if (columnPatternMatcher == null) return columns; swap these two lines and return Collections.emptyList(); http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS8, Line 211: params Plz don't reference variable names from the body of this function in the function comment. I had to look inside the function to figure out what params is. PS8, Line 266: params Same comment as above. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS8, Line 226: Class contains all information needed by getMetadataOp(). "Utility class that represents the input parameters of getDbsMetadata() function calls." PS8, Line 286: the : * search pattern "the associated search patterns" PS8, Line 287: metadataParams 'metadataParams'. PS8, Line 287: JDBC search patterns You may want to add a ref to the PatternMatcher class so that the reader can see what a JDBC pattern matcher does. PS8, Line 289: The return value result.dbs contains the list of databases that satisfy : * the "dbPattern_" search pattern. : * result.tableNames[i] contains the list of tables inside dbs[i] that satisfy : * the "tablePattern_" search pattern. : * result.columns[i][j] contains the list of columns of table[j] in dbs[i] : * that satisfy the search condition "columnPattern_". : * result.functions[i] contains the list of functions inside dbs[i] that : * satisfy the "functionPattern_" search pattern. You need to update this comment. e.g. "dbPattern_" doesn't even show up in this function anymore. PS8, Line 348: if (columnPatternMatcher == null) continue; I don't think that is correct here. You'll never populate the missingTbls unless a column pattern is specified. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: Line 1543: Maybe also add a call to getDbs where the pattern matches a db for which the user doesn't have any permissions on, hence an empty list is returned. PS8, Line 1626: same as "%" Also indicate what that means (.e.g. matches all). PS8, Line 1641: Pattern ".*" or "." works as well Mention what that matches. Here and everywhere else PS8, Line 1657: "_" should work Not very descriptive comment :) http://gerrit.cloudera.org:8080/#/c/3371/8/testdata/workloads/functional-query/queries/QueryTest/show.test File testdata/workloads/functional-query/queries/QueryTest/show.test: PS8, Line 157: # This behaves different than Hive, 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 What are we testing with these cases? -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
