Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13955 )
Change subject: IMPALA-8755: Frontend support for Z-ordering ...................................................................... Patch Set 8: (13 comments) Had a couple of minor comments, nothing serious. Otherwise looks pretty good. http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG@12 PS8, Line 12: algorithm nit: order http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG@21 PS8, Line 21: queries statements http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG@25 PS8, Line 25: For strings, varchars, floats and doubles Z-ordering is not supported. You could mention that zorder is not suitable for string and varchars, but support for floats and doubles can be added later. http://gerrit.cloudera.org:8080/#/c/13955/8/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/8/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@221 PS8, Line 221: AlterTableSortByStmt.TBL_PROP_SORT_ORDER)); nit: missing spaces from the beginning http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java: http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java@37 PS8, Line 37: ZORDER nit: ORDER, or LEXICAL|ZORDER http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java@65 PS8, Line 65: nit: extra space http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java: http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java@51 PS8, Line 51: private final TSortingOrder sortingOrder_; nit: you could place it after 'sortColumns_' http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java@64 PS8, Line 64: List of nit: Pair of ... http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@113 PS8, Line 113: public TSortingOrder getSortingOrder() { : return tableDef_.getSortingOrder(); : } nit: fits single line http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@62 PS8, Line 62: // Sort using Z-ordering order nit: comment is obsolete http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@103 PS8, Line 103: //if (sortingOrder_ == null) return TSortingOrder.LEXICAL; nit: some old code has been left in the comment, please remove http://gerrit.cloudera.org:8080/#/c/13955/8/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by-zorder.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by-zorder.test: http://gerrit.cloudera.org:8080/#/c/13955/8/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by-zorder.test@424 PS8, Line 424: order by: ZORDER: id are these tests still succeed? I think they should because we have a constant 'false' for 'bool_col' and it seems like a reasonable optimization. http://gerrit.cloudera.org:8080/#/c/13955/8/testdata/workloads/functional-query/queries/QueryTest/create-table-zorder.test File testdata/workloads/functional-query/queries/QueryTest/create-table-zorder.test: http://gerrit.cloudera.org:8080/#/c/13955/8/testdata/workloads/functional-query/queries/QueryTest/create-table-zorder.test@3 PS8, Line 3: # Make sure that specifying sort by zorder columns sets the 'sort.columns' nit: whitespace at eol -- 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: 8 Gerrit-Owner: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 13 Aug 2019 17:16:06 +0000 Gerrit-HasComments: Yes
