Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/13955 )
Change subject: IMPALA-8755: Frontend support for Z-ordering ...................................................................... Patch Set 9: (14 comments) Thank you for the feedback, added some comments, and changed some code. http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG@9 PS9, Line 9: , > nit: is this comma needed? Done http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG@25 PS9, Line 25: For strings, varchars, floats and doubles Z-ordering is currently : not supported. > nit: Could you also state for which type(s) this is supported? Is it just I Everything not mentioned are supported. http://gerrit.cloudera.org:8080/#/c/13955/9//COMMIT_MSG@30 PS9, Line 30: unlock_zorder > enable_zorder_sort or such seems better for me. For me, 'unlock' sounds more like the word I'd use with an experimental feature, and 'enable' with some already implemented addition. Maybe unlock_zorder_sort would be better? 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 > What if the source table used ZORDER sorting? Can "CREATE TABLE LIKE" state The null passed previously was the the sorting columns, therefore in this case no sorting property was inherited. To add it again, someone has to add a SORT BY [ZORDER] clause, which will also define the ordering. http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/cup/sql-parser.cup@1479 PS9, Line 1479: TSortingOrder.LEXICAL > If there are no columns specified to sort by then does it make sense to say NOT_APPLICABLE is also an option, since any new sorting setting would requires a SORT BY clause and therefore it would be set properly. However, for me, the default value or just null might sound better. 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@50 PS9, Line 50: import com.google.common.collect.ImmutableList; > Do you use this in this file? Done http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@213 PS9, Line 213: throw new AnalysisException(String.format("'%s' or '%s' table property is not " + : "supported for Kudu tables.", AlterTableSortByStmt.TBL_PROP_SORT_COLUMNS, : AlterTableSortByStmt.TBL_PROP_SORT_ORDER)); > Not sure if this is something requested by someone, but I don't think exten As far as I understand, this function is called when the 'sort.columns' or the 'sort.order' properties are set manually. (See at the comment before the class.) We could write something like 'sort.*' properties are not supported for Kudu tables. 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@154 PS9, Line 154: this.sortingOrder = sortProperties.second; > nit: Could you move this next to this.sortCols? Then both the usage of sort Done 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())); > What if multiple non-supported types are provided? Do we get an exception f Yes, it raises an exception when finds the first one. Would it be better to scan all columns for unsupported types and return them all? 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@68 PS9, Line 68: ,e > nit: missing space Done 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 > nit: This is needed only at L376, right? Makes sense to move it closer. Most of the variables in this function are only needed in the return clause and not used anywhere else, so I'd keep it next to the other sorting related variable. http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java File fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@70 PS9, Line 70: sortingOrder > nit: If 'sortingOrder_' is... Done http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@70 PS9, Line 70: If sortingOrder is not lexicographical, the backend will skip this process. > nit: Is this statement true until you deliver the BE implementation as well The process is skipped, because when the order is not lexicographical, the parquet files won't be sorted in the usual manner, so the sortColumns_ variable won't actually store sorting columns (as parquet would expect them to be). http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/13955/9/fe/src/main/java/org/apache/impala/planner/SortNode.java@211 PS9, Line 211: switch (info_.getSortingOrder()) { : case LEXICAL: : for (int i = 0; i < info_.getSortExprs().size(); ++i) { : if (i > 0) output.append(", "); : output.append(info_.getSortExprs().get(i).toSql() + " "); : output.append(info_.getIsAscOrder().get(i) ? "ASC" : "DESC"); : : Boolean nullsFirstParam = info_.getNullsFirstParams().get(i); : if (nullsFirstParam != null) { : output.append(nullsFirstParam ? " NULLS FIRST" : " NULLS LAST"); : } : } : break; : case ZORDER: : output.append("ZORDER: "); : for (int i = 0; i < info_.getSortExprs().size(); ++i) { : if (i > 0) output.append(", "); : output.append(info_.getSortExprs().get(i).toSql()); : } : break; > This look very similar to what ExchangeNode.getNodeExplainString() does. Is Merged the two in a function in the base class. -- 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: 9 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: Thu, 22 Aug 2019 10:58:39 +0000 Gerrit-HasComments: Yes
