Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata() ......................................................................
Patch Set 13: (11 comments) thanks Henry and Dimitris 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 PatternM Setting the pattern of the matcher to null is the problem we are trying to solve. I can push the logic to filterCatalogObjectsByPattern and filterStringsByPattern though. what do you think PS13, Line 226: See filterStringsByPattern : * for details of the pattern match semantics. > is this comment stale now? Done PS13, Line 231: getDataSourceNames > Is this version really necessary? We don't have this specialisation for ge what do you mean by version? I changed this code because I changed the interface of filterStringsByPattern to take a pattern matcher? PS13, Line 237: aal > all Done PS13, Line 237: the pattern of 'matcher'. > nit, just say 'that match 'matcher'' Done PS13, Line 238: @see PatternMatcher for supported matchers > this seems redundant, and can be removed. Done, and I changed all occurrence. 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 shou Done PS13, Line 334: private > probably should live in PatternMatcher itself. then the same may apply to filterCatalogObjectsByPattern(), but it accesses getName(). So I guess it is better to take a filterKey functor as input? so it can call getName() accordingly? But I think that is unnecessary refactor? what do you think? PS13, Line 360: f : * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returne > same comment as above Done PS13, Line 365: @see PatternMatcher#matches(String) for matching mechanisms. > I don't think this is necessary. Callers of this method will have construct Done 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 null is a valid pattern. so we cannot tell if a null pattern is set by the caller or it is just unset. This is one of the problem we are trying to solve. -- 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
