impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Date Wed, 29 Jun 2016 01:06:55 GMT
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 <hxu@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message