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: (13 comments) 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: order is > nit: order Done http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG@21 PS8, Line 21: stateme > statements Done http://gerrit.cloudera.org:8080/#/c/13955/8//COMMIT_MSG@25 PS8, Line 25: For strings, varchars, floats and doubles Z-ordering is currently > You could mention that zorder is not suitable for string and varchars, but Done 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 Done 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: LEXICA > nit: ORDER, or LEXICAL|ZORDER Done 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 Done 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 boolean ifNotExists_; > nit: you could place it after 'sortColumns_' Done http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java@64 PS8, Line 64: A pair, > nit: Pair of ... Done 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 String getComment() { return tableDef_.getComment(); } : Map<String, String> getTblProperties() { return tableDef_.getTblProperties(); } : p > nit: fits single line Done 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: private TSortingOrder sortingO > nit: comment is obsolete Done http://gerrit.cloudera.org:8080/#/c/13955/8/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@103 PS8, Line 103: * Gets the list of booleans indicating whether nulls come f > nit: some old code has been left in the comment, please remove Done 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 consta yes, the test succeeds, the optimisation worked without any additional change 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 Done -- 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: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 14 Aug 2019 09:33:51 +0000 Gerrit-HasComments: Yes
