You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Adam Holley (Code Review)" <ge...@cloudera.org> on 2018/02/10 01:49:25 UTC
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Adam Holley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9276
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
12 files changed, 307 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/9276/1
--
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: newchange
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 05:59:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley 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:
(7 comments)
Fixed.
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& TSes
Done
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-intern
Done
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:
Done
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, irres
Done
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'
Done
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.
Done
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.
Done
--
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 <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 18:39:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Reviewed-on: http://gerrit.cloudera.org:8080/9276
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 324 insertions(+), 67 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 333 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/9276/2
--
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: newpatchset
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 325 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/9276/4
--
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: newpatchset
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 1:
Original patch build: https://jenkins.impala.io/job/gerrit-verify-dryrun-external/80/
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 17:36:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 1:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/client-request-state.cc@381
PS1, Line 381: params->__isset.table_name ? &(params->table_name) : NULL;
NULL -> nullptr
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/frontend.h@133
PS1, Line 133: Status DescribeTable(const TDescribeOutputStyle::type output_style,
Why do we need to change the signature so dramatically? Should it not be sufficient to have:
Status DescribeTable(const TDescribeTableParams& params, const TSessionState& session, TDescribeResult* response);
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift@173
PS1, Line 173: 4: optional ImpalaInternalService.TSessionState session
I don't think this is needed.
The session state is available in the BE during:
ClientRequestState::ExecLocalCatalogOp()
You can pass 'query_ctx_.session' to frontend_->DescribeTable()
http://gerrit.cloudera.org:8080/#/c/9276/1/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/1/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java@122
PS1, Line 122: resultStruct_ = Path.getTypeAsStruct(path_.destType());
Maybe I'm missing something, but it seems like the following scenario is not authorized properly:
create table default.t (
id int,
a array<struct<f1:int,f2:string>>
)
User has column-level privileges on 'id', but not on 'a'.
DESCRIBE default.t.a
That describe should fail, but I think it will succeed.
This case needs a test.
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java@489
PS1, Line 489: public StructType getHiveColumnsAsStruct(List<Column> columns) {
Seems weird to have this as a member, since it's totally non-specific to this Table.
How about making this a static function in Column like columnsToStruct()
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java@490
PS1, Line 490: ArrayList<StructField> fields = Lists.newArrayListWithCapacity(colsByPos_.size());
columns.size()
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
File fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@199
PS1, Line 199: List<Column> nonClustered = new ArrayList<Column>(table.getNonClusteringColumns());
Will this code work if 'nonClustered' or 'clustered' is empty?
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@200
PS1, Line 200: nonClustered.retainAll(filteredColumns);
Concise but slow. I suggest this instead
List<Column> nonClustered
List<Column> clustered
for (Column c: filteredColumns) {
if (table.isClusteringColumn(c) {
clustered.add
} else {
nonClustered.add
}
}
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@259
PS1, Line 259: * Builds a TDescribeResult for a table.
update comment
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@261
PS1, Line 261: public static TDescribeResult buildDescribeMinimalResult(List<Column> columns) {
buildKuduDescribeMinimalResult()
http://gerrit.cloudera.org:8080/#/c/9276/1/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/1/fe/src/main/java/org/apache/impala/service/Frontend.java@796
PS1, Line 796: for (Column col: table.getColumnsInHiveOrder()) {
Can we do a table-level check first? Checking all columns all the time is pretty expensive.
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/Frontend.java@806
PS1, Line 806: filteredColumns = table.getColumns();
Shouldn't this be getColumnsInHiveOrder()?
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@434
PS1, Line 434: // If the session was not set it indicates this is an internal Impala call.
Where is this called internally? I only see this function being called ClientRequestState::ExecLocalCatalogOp().
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@116
PS1, Line 116: private static final List<String> FUNCTIONAL_DESCRIBE_DATA_ALLTYPESAGG =
* Move these closer to where they are used (near the test)
* How about EXPECTED_DESCRIBE_ALLTYPESAGG. It doesn't make sense to me to have FUNCTIONAL at the beginning and then ALLTYPESAGG at the end. We can add a comment that these refer to tables in the "functional" database.
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@134
PS1, Line 134: private static final List<String> FUNCTIONAL_DESCRIBE_DATA_ALLTYPESSMALL =
EXPECTED_DESCRIBE_ALLTYPESSMALL
(and similar pattern for the names below)
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1785
PS1, Line 1785: public void TestDescribeTableResults() throws ImpalaException {
Move this below TestDescribe() to keep them together
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1787
PS1, Line 1787: TDescribeResult result = fe_.describeTable(new TTableName("functional","alltypesagg"),
No test for filtering the view text
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1816
PS1, Line 1816: // This method compares two arrays but skips an expected value that contains '*' since
// Compares ...
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1820
PS1, Line 1820: for(int idx=0; idx<expected.size(); idx++) {
style: spaces missing after "for", before and after "=" and "<"
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1827
PS1, Line 1827: // Private method convert TDescribeResult to ArrayList of strings.
"Private method" is redundant, remove
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1830
PS1, Line 1830: for(TResultRow row: result.getResults()) {
style: space after "for"
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1990
PS1, Line 1990: public void TestHs2GetColumns() throws ImpalaException {
Out of scope of this JIRA?
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 22:49:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 22:02:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift@173
PS1, Line 173: 4: optional ImpalaInternalService.TSessionState session
> Since it's not the only parameter, if I need to pass the session through th
Yes, you are right. We need to keep this field.
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 18:08:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 4:
(2 comments)
Updated comments.
http://gerrit.cloudera.org:8080/#/c/9276/4/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/9276/4/be/src/service/frontend.h@128
PS4, Line 128: /// properties, and more. If this is a user initiated request, it should
> This is always a user initiated request, don't think we need that comment
Done
http://gerrit.cloudera.org:8080/#/c/9276/4/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/4/fe/src/main/java/org/apache/impala/service/Frontend.java@830
PS4, Line 830: // Filter out LOCATION and VIEW text
> remove dead filtering code
Done
--
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: 4
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 21:29:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 327 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/9276/3
--
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: newpatchset
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
IMPALA-6479: Update DESCRIBE to respect column privileges
Modified the Frontend to filter columns from the DESCRIBE
statement. Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.
Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.
Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 324 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/9276/5
--
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: newpatchset
Gerrit-Change-Id: Ic96ae184fccdc88ba970b5adcd501da1966accb9
Gerrit-Change-Number: 9276
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 4:
(2 comments)
lgtm after these final comments
http://gerrit.cloudera.org:8080/#/c/9276/4/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/9276/4/be/src/service/frontend.h@128
PS4, Line 128: /// properties, and more. If this is a user initiated request, it should
This is always a user initiated request, don't think we need that comment
http://gerrit.cloudera.org:8080/#/c/9276/4/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/4/fe/src/main/java/org/apache/impala/service/Frontend.java@830
PS4, Line 830: // Filter out LOCATION and VIEW text
remove dead filtering code
--
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: 4
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 21:08:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 1:
(22 comments)
Changes applied.
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/client-request-state.cc@381
PS1, Line 381: params->__isset.table_name ? &(params->table_name) : NULL;
> NULL -> nullptr
Removed lines from other refactoring.
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/frontend.h
File be/src/service/frontend.h:
http://gerrit.cloudera.org:8080/#/c/9276/1/be/src/service/frontend.h@133
PS1, Line 133: Status DescribeTable(const TDescribeOutputStyle::type output_style,
> Why do we need to change the signature so dramatically? Should it not be su
Refactored.
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift@173
PS1, Line 173: 4: optional ImpalaInternalService.TSessionState session
> Yes, you are right. We need to keep this field.
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/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/1/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java@122
PS1, Line 122: resultStruct_ = Path.getTypeAsStruct(path_.destType());
> Maybe I'm missing something, but it seems like the following scenario is no
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java@489
PS1, Line 489: public StructType getHiveColumnsAsStruct(List<Column> columns) {
> Seems weird to have this as a member, since it's totally non-specific to th
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/catalog/Table.java@490
PS1, Line 490: ArrayList<StructField> fields = Lists.newArrayListWithCapacity(colsByPos_.size());
> columns.size()
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
File fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@199
PS1, Line 199: List<Column> nonClustered = new ArrayList<Column>(table.getNonClusteringColumns());
> Will this code work if 'nonClustered' or 'clustered' is empty?
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@200
PS1, Line 200: nonClustered.retainAll(filteredColumns);
> Concise but slow. I suggest this instead
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@259
PS1, Line 259: * Builds a TDescribeResult for a table.
> update comment
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java@261
PS1, Line 261: public static TDescribeResult buildDescribeMinimalResult(List<Column> columns) {
> buildKuduDescribeMinimalResult()
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/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/1/fe/src/main/java/org/apache/impala/service/Frontend.java@796
PS1, Line 796: for (Column col: table.getColumnsInHiveOrder()) {
> Can we do a table-level check first? Checking all columns all the time is p
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/Frontend.java@806
PS1, Line 806: filteredColumns = table.getColumns();
> Shouldn't this be getColumnsInHiveOrder()?
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@434
PS1, Line 434: // If the session was not set it indicates this is an internal Impala call.
> Where is this called internally? I only see this function being called Clie
Done
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@116
PS1, Line 116: private static final List<String> FUNCTIONAL_DESCRIBE_DATA_ALLTYPESAGG =
> * Move these closer to where they are used (near the test)
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@134
PS1, Line 134: private static final List<String> FUNCTIONAL_DESCRIBE_DATA_ALLTYPESSMALL =
> EXPECTED_DESCRIBE_ALLTYPESSMALL
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1785
PS1, Line 1785: public void TestDescribeTableResults() throws ImpalaException {
> Move this below TestDescribe() to keep them together
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1787
PS1, Line 1787: TDescribeResult result = fe_.describeTable(new TTableName("functional","alltypesagg"),
> No test for filtering the view text
Column-level privileges on views are not currently supported. I can remove the code, but as it was part of the original request, I'd prefer to leave it in at such time column level privileges on views are added.
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1816
PS1, Line 1816: // This method compares two arrays but skips an expected value that contains '*' since
> // Compares ...
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1820
PS1, Line 1820: for(int idx=0; idx<expected.size(); idx++) {
> style: spaces missing after "for", before and after "=" and "<"
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1827
PS1, Line 1827: // Private method convert TDescribeResult to ArrayList of strings.
> "Private method" is redundant, remove
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1830
PS1, Line 1830: for(TResultRow row: result.getResults()) {
> style: space after "for"
Done
http://gerrit.cloudera.org:8080/#/c/9276/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1990
PS1, Line 1990: public void TestHs2GetColumns() throws ImpalaException {
> Out of scope of this JIRA?
Done
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Mar 2018 19:20:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 14 Mar 2018 01:45:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 1:
(1 comment)
Quick clarification.
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9276/1/common/thrift/Frontend.thrift@173
PS1, Line 173: 4: optional ImpalaInternalService.TSessionState session
> I don't think this is needed.
Since it's not the only parameter, if I need to pass the session through the JNI call, doesn't it need to be added? getTableName and getDbs both have it as parameters.
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 21:10:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2094/
--
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: 5
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 13 Mar 2018 22:03:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6479: Update DESCRIBE to respect column privileges
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9276 )
Change subject: IMPALA-6479: Update DESCRIBE to respect column privileges
......................................................................
Patch Set 1:
(1 comment)
Removed code for view.
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: TDescribeResult result = fe_.describeTable(new TTableName("functional","alltypesagg"),
> I don't think we should check in dead code that will only confuse readers,
I'll remove.
--
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: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 18:57:46 +0000
Gerrit-HasComments: Yes