Huaisi Xu 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" Done PS8, Line 13: user only request database name > The description is a bit misleading. In general, requesting a db from the c I will update this later. I need to think about this. 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 commen Done PS8, Line 134: matcher must not be null. > You don't enforce that. Add a Preconditions.checkNotNull() above L137 or re Done PS8, Line 172: tablePatternMatcher > Just rename to "matcher". Done PS8, Line 359: Filter candidates.getName() using matcher > "Return all catalog objects from 'candidates' that match the pattern of 'ma Done 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. yeah I noticed this.. my IDE changed this.. PS8, Line 427: Returns all functions using fnPatternMatcher > "Returns all functions that match the pattern of 'matcher'. if 'matcher' is Done 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 Done PS8, Line 595: Throw ImpalaException when dbName is null > Where does this happen? Also when describing the conditions during which an ok. just removed. it happens in getTableNames(dbName, matcher). PS8, Line 598: tablePatternMatcher > Let's rename all these variables to 'matcher' unless there are multiple ins Done PS8, Line 624: List<Column> columns = Lists.newArrayList(); : if (columnPatternMatcher == null) return columns; > swap these two lines and return Collections.emptyList(); Done 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 fu Done PS8, Line 266: params > Same comment as above. Done 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() fun Done PS8, Line 287: JDBC search patterns > You may want to add a ref to the PatternMatcher class so that the reader ca Done PS8, Line 287: metadataParams > 'metadataParams'. Done PS8, Line 286: the : * search pattern > "the associated search patterns" Done 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 I was referring to members in metadataParams. Done. PS8, Line 348: if (columnPatternMatcher == null) continue; > I don't think that is correct here. You'll never populate the missingTbls u Agree.. I changed the behavior of this function. thanks! 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 th Done PS8, Line 1626: same as "%" > Also indicate what that means (.e.g. matches all). Done PS8, Line 1641: Pattern ".*" or "." works as well > Mention what that matches. Here and everywhere else Done PS8, Line 1657: "_" should work > Not very descriptive comment :) Done 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? I just want to fix the behavior of our end-to-end result. Unit test is only for integrity of those particular methods, and they work as expected. but end-to-end is not necessary. Hive' behavior is different. -- 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
