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 <g...@holleyism.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Feb 2018 22:49:11 +0000 Gerrit-HasComments: Yes
