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: (4 comments) 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: f stats-related varia > nit: What about getSortingOrderExplainString() ? Done 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"? It should be correct with "Baz" (queries taken from the SORT BY tests) 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 Added extra comment with TODO, this test can be merged with other sort by tests when Z-ordering won't be hidden under a feature flag. 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 overwh The function and the comment was copied from the file mentioned above. Since it's only temporary, I would not leave it that way. -- 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 <norbert.lu...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@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, 03 Sep 2019 15:22:42 +0000 Gerrit-HasComments: Yes
