Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13955 )
Change subject: IMPALA-8755: Frontend support for Z-ordering ...................................................................... Patch Set 11: (10 comments) http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG@30 PS9, Line 30: > For me, 'unlock' sounds more like the word I'd use with an experimental fea hmm, indeed, unlock_zorder_sort would be slightly better. (nit) http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/cup/sql-parser.cup@1399 PS9, Line 1399: CreateTableLikeStmt > The null passed previously was the the sorting columns, therefore in this c Thx for the explanation! http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/cup/sql-parser.cup@1479 PS9, Line 1479: TSortingOrder.LEXICAL > NOT_APPLICABLE is also an option, since any new sorting setting would requi Ok, I'm fine with that. http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@213 PS9, Line 213: "supported for Kudu tables."); : } : > As far as I understand, this function is called when the 'sort.columns' or I see. It's fine as it is. http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/TableDef.java@397 PS9, Line 397: throw new AnalysisException(String.format("SORT BY ZORDER does not support " : + "column type: %s", colType.toSql())); > Yes, it raises an exception when finds the first one. Would it be better to Hmm, It might be more usable if you printed out all the unsupported columns here as users would have to run multiple loops of query rewrite - re-execution if the run this on multiple unsupported columns. As I see this is not a listing of particular columns just the unsupported types so it won't overwhelm the output if we printed all of them here. What do you think? http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@315 PS9, Line 315: TSortingOrder sortingOrder = TSortingOrder.valueOf(getSortingOrder(properties)); > Most of the variables in this function are only needed in the return clause Sure http://gerrit.cloudera.org:8080/#/c/13955/11/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/13955/11/fe/src/main/java/org/apache/impala/planner/PlanNode.java@641 PS11, Line 641: getOrderExplainString nit: What about getSortingOrderExplainString() ? http://gerrit.cloudera.org:8080/#/c/13955/11/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/13955/11/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@2635 PS11, Line 2635: Baz nit: Baz? Isn't this meant to be "Bar"? http://gerrit.cloudera.org:8080/#/c/13955/11/tests/custom_cluster/test_zorder.py File tests/custom_cluster/test_zorder.py: http://gerrit.cloudera.org:8080/#/c/13955/11/tests/custom_cluster/test_zorder.py@29 PS11, Line 29: Temporary Could you explain why this is temporary? If this goes away when the rest of the feature arrives could you add a TODO here? http://gerrit.cloudera.org:8080/#/c/13955/11/tests/custom_cluster/test_zorder.py@122 PS11, Line 122: Runs a show-create-table test file, containing the following sections: : : ---- CREATE_TABLE : contains a table creation statement to create table TABLE_NAME : ---- RESULTS : contains the expected result of SHOW CREATE TABLE table_name : : OR : : ---- CREATE_VIEW : contains a view creation statement to create table VIEW_NAME : ---- RESULTS : contains the expected result of SHOW CREATE VIEW table_name : : OR : : ---- QUERY : a show create table query : ---- RESULTS : contains the expected output of the SHOW CREATE TABLE query nit: Could you re-phrase this function comment? I feel this is a bit overwhelming and could be expressed with some easy to read sentences. -- To view, visit http://gerrit.cloudera.org:8080/13955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie122002ca8f52ca2c1e1ec8ff1d476ae1f4f875d Gerrit-Change-Number: 13955 Gerrit-PatchSet: 11 Gerrit-Owner: Norbert Luksa <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2019 09:58:52 +0000 Gerrit-HasComments: Yes
