Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9276 )

Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/9276/2/be/src/service/frontend.h
File be/src/service/frontend.h:

http://gerrit.cloudera.org:8080/#/c/9276/2/be/src/service/frontend.h@129
PS2, Line 129:   /// be set to the user's current session. If this is an Impala 
internal request,
Fix comment regarding "Impala internal calls". We should pass a const& 
TSessionState.

We usually prefer const& for input params and pointers for output params. We're 
not religiously consistent, but I think in this case it's easy to change to 
const TSessionState&


http://gerrit.cloudera.org:8080/#/c/9276/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/9276/2/common/thrift/Frontend.thrift@171
PS2, Line 171:   // enabled, only the functions this user has access to will be 
returned. If not
Fix comment. We're not returning functions, and we don't have Impala-internal 
requests for describe.


http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java@121
PS2, Line 121:     if (path_.destColumn() != null) {
We're missing a privilege request for the following case:

describe functional.allcomplextypes.nested_struct_col.f1

* destColumn() is null
* destType() is not complex

I think this code can be condensed/simplified, see comment below.

We need tests for all these code paths.


http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java@128
PS2, Line 128:       analyzer.registerPrivReq(new PrivilegeRequestBuilder()
I think this request should be unconditionally registered after L119, 
irrespective of destColumn() and destType()


http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/service/Frontend.java@805
PS2, Line 805:               .onColumn(table.getDb().getName(), 
table.getName(),colName)
space before 'colName'


http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/service/Frontend.java@812
PS2, Line 812:         filteredColumns = table.getColumnsInHiveOrder();
// User has table-level access.


http://gerrit.cloudera.org:8080/#/c/9276/2/fe/src/main/java/org/apache/impala/service/Frontend.java@815
PS2, Line 815:       filteredColumns = table.getColumnsInHiveOrder();
// Authorization is disabled.


http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1787
PS1, Line 1787:     assertEquals(expectedDbs, extractDbNames(dbs));
> Column-level privileges on views are not currently supported.  I can remove
I don't think we should check in dead code that will only confuse readers, 
reviewers and code coverage tools.

Let's add the code and tests when we actually have support for that feature.



--
To view, visit http://gerrit.cloudera.org:8080/9276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <g...@holleyism.com>
Gerrit-Reviewer: Adam Holley <g...@holleyism.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 05:59:29 +0000
Gerrit-HasComments: Yes

Reply via email to