Alex Behm has posted comments on this change. ( )

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

Patch Set 2:

File be/src/service/frontend.h:
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& 

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&
File common/thrift/Frontend.thrift:
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.
File fe/src/main/java/org/apache/impala/analysis/
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.
PS2, Line 128:       analyzer.registerPrivReq(new PrivilegeRequestBuilder()
I think this request should be unconditionally registered after L119, 
irrespective of destColumn() and destType()
File fe/src/main/java/org/apache/impala/service/
PS2, Line 805:               .onColumn(table.getDb().getName(), 
space before 'colName'
PS2, Line 812:         filteredColumns = table.getColumnsInHiveOrder();
// User has table-level access.
PS2, Line 815:       filteredColumns = table.getColumnsInHiveOrder();
// Authorization is disabled.
File fe/src/test/java/org/apache/impala/analysis/
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Adam Holley <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Comment-Date: Tue, 13 Mar 2018 05:59:29 +0000
Gerrit-HasComments: Yes

Reply via email to