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 <g...@holleyism.com> Gerrit-Reviewer: Adam Holley <g...@holleyism.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Mar 2018 19:20:37 +0000 Gerrit-HasComments: Yes
