Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata() ......................................................................
Patch Set 9: (10 comments) Minor comment. Getting there. http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS9, Line 235: using matcher "that match the pattern of 'matcher'." PS9, Line 238: matcher nit: 'matcher'; good practice to use single quotes when referring to input params. PS9, Line 328: candidats typo: candidates http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS9, Line 614: columnPatternMatcher update PS9, Line 615: columnPatternMatcher 'matcher' http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS9, Line 210: Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] '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. Can you move this below L220? The first sentence of the function comment should describe the main role of a function. PS9, Line 265: Implement Hive's pattern-matching semantics for "SHOW DATABASES [[LIKE] '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. This is kind of repetitive. You may want to keep the first sentence, move it below L275 and remove the rest. If you want you may point to the function comment of getTableNames above. http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS9, Line 226: getMetadataOp There is no such thing :) PS9, Line 302: If metadataParams.tablePattern_ is null, then result.tableNames and result.columns : * will not be populated. : * If metadataParams.columnPattern_ is null, then result.columns will not be populated. That's not correct. tablePattern_ could be set to null which indicates that everything matches. Same for columnPattern_, if it is null it means that all columns match for the matching dbs and tables. http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java: PS9, Line 34: Returns true if pattern_ is null or the candidate matches. : // Returns false if pattern_ is empty or the candidate mismatches. Needless to say, these semantics are really weird. Btw, you have a typo, it's patterns_ not pattern_. -- 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: 9 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
