Henry Robinson has posted comments on this change. Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata() ......................................................................
Patch Set 13: (11 comments) http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS13, Line 175: if (matcher == null) return Collections.emptyList(); rather than dealing with this case everywhere, push the logic into PatternMatcher. This is probably easiest achieved by setting the pattern of the matcher to null. PS13, Line 226: See filterStringsByPattern : * for details of the pattern match semantics. is this comment stale now? PS13, Line 231: getDataSourceNames Is this version really necessary? We don't have this specialisation for getTableNames or getDbNames I think. PS13, Line 237: the pattern of 'matcher'. nit, just say 'that match 'matcher'' PS13, Line 237: aal all PS13, Line 238: @see PatternMatcher for supported matchers this seems redundant, and can be removed. PS13, Line 327: If : * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returned. This comment leaks through the abstraction of PatternMatcher - i.e. it should be sufficient to say "Return all members of 'candidates' that match 'matches'". PS13, Line 334: private probably should live in PatternMatcher itself. PS13, Line 360: f : * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returne same comment as above PS13, Line 365: @see PatternMatcher#matches(String) for matching mechanisms. I don't think this is necessary. Callers of this method will have constructed a PatternMatcher, so know that that's where they should look for matching semantics. http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS13, Line 274: public boolean hasDbPattern() { return isDbPatternSet_; } : public boolean hasTablePattern() { return isTablePatternSet_; } : public boolean hasColumnPattern() { return isColumnPatternSet_; } : public boolean hasFunctionPattern() { return isFunctionPatternSet_; } it would be cleaner to make it so that the default values (maybe null?) do the right thing, so you don't have to write any logic to handle the special case where the pattern is not set. -- 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: 13 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
