Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201606282326.u5SNQZ3p008446@ip-10-146-233-104.ec2.internal>
Date: Tue, 28 Jun 2016 23:26:35 +0000
From: "Huaisi Xu (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Reply-To: hxu@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-CR=5D=28cdh5-trunk=29_IMPALA-3711=3A_Check_privileges_when_necessary=0A?=
In-Reply-To: <gerrit.1465605484000.I17d8c5b9fb12483e4b01b819fba48b6849311a14@gerrit.cloudera.org>
References: <gerrit.1465605484000.I17d8c5b9fb12483e4b01b819fba48b6849311a14@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Tue, 28 Jun 2016 23:26:40 -0000

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 <hxu@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-HasComments: Yes