Alex Behm has posted comments on this change. ( )

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

Patch Set 1:

File be/src/service/
PS1, Line 381:           params->__isset.table_name ? &(params->table_name) : 
NULL -> nullptr
File be/src/service/frontend.h:
PS1, Line 133:   Status DescribeTable(const TDescribeOutputStyle::type 
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);
File common/thrift/Frontend.thrift:
PS1, Line 173:   4: optional ImpalaInternalService.TSessionState session
I don't think this is needed.

The session state is available in the BE during:

You can pass 'query_ctx_.session' to frontend_->DescribeTable()
File fe/src/main/java/org/apache/impala/analysis/
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.
File fe/src/main/java/org/apache/impala/catalog/
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 

How about making this a static function in Column like columnsToStruct()
PS1, Line 490:     ArrayList<StructField> fields = 
File fe/src/main/java/org/apache/impala/service/
PS1, Line 199:     List<Column> nonClustered = new 
Will this code work if 'nonClustered' or 'clustered' is empty?
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) {
  } else {
PS1, Line 259:    * Builds a TDescribeResult for a table.
update comment
PS1, Line 261:   public static TDescribeResult 
buildDescribeMinimalResult(List<Column> columns) {
File fe/src/main/java/org/apache/impala/service/
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.
PS1, Line 806:       filteredColumns = table.getColumns();
Shouldn't this be getColumnsInHiveOrder()?
File fe/src/main/java/org/apache/impala/service/
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 
File fe/src/test/java/org/apache/impala/analysis/
PS1, Line 116:   private static final List<String> 
* 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.
PS1, Line 134:   private static final List<String> 

(and similar pattern for the names below)
PS1, Line 1785:   public void TestDescribeTableResults() throws ImpalaException 
Move this below TestDescribe() to keep them together
PS1, Line 1787:     TDescribeResult result = fe_.describeTable(new 
No test for filtering the view text
PS1, Line 1816:   // This method compares two arrays but skips an expected 
value that contains '*' since
// Compares ...
PS1, Line 1820:     for(int idx=0; idx<expected.size(); idx++) {
style: spaces missing after "for", before and after "=" and "<"
PS1, Line 1827:   // Private method convert TDescribeResult to ArrayList of 
"Private method" is redundant, remove
PS1, Line 1830:     for(TResultRow row: result.getResults()) {
style: space after "for"
PS1, Line 1990:   public void TestHs2GetColumns() throws ImpalaException {
Out of scope of this JIRA?

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: 1
Gerrit-Owner: Adam Holley <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Comment-Date: Mon, 12 Feb 2018 22:49:11 +0000
Gerrit-HasComments: Yes

Reply via email to