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