Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 5: (5 comments) Responses to some comments. http://gerrit.cloudera.org:8080/#/c/6495/5/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: PS5, Line 455: the caller must make sure that the value matches any columns he/she adds to the : * table. > We could throw one if the number is wrong, but eventually the caller will h No that's fine. You already mention that this is used only for tests. http://gerrit.cloudera.org:8080/#/c/6495/5/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS5, Line 546: orderingExprs.addAll(insertStmt.getPartitionKeyExprs() > Insert hints are not allowed for HBase tables and the analysis will fail. F If we enforce it at the analysis phase, then adding a Preconditions check here is not a bad idea. http://gerrit.cloudera.org:8080/#/c/6495/5/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test: PS5, Line 10: ASC NULLS LAST > No, we cannot change it. I thought about this, but couldn't see why we'd wa I was thinking more of changing the ASC into DESC but that's fine if this is an acceptable limitation. PS5, Line 39: ---- DISTRIBUTEDPLAN : WRITE TO HDFS [test_sort_by.alltypes, OVERWRITE=false, PARTITION-KEYS=(year,month)] : | partitions=24 : | : 01:SORT : | order by: year ASC NULLS LAST, month ASC NULLS LAST, int_col ASC NULLS LAST, bool_col ASC NULLS LAST : | : 00:SCAN HDFS [functional.alltypes] : partitions=24/24 files=24 size=478.45KB > They make sure that the shuffle/noshuffle hint has been observed, indicated Good point. I think we can leave them. http://gerrit.cloudera.org:8080/#/c/6495/5/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: PS5, Line 1066: > They are part of the output of "describe formatted". Without them the compa It's probably because of the HMS schema used (maybe they use CHAR(N)). Anyways, no need to do something. I just wanted to make sure we don't do anything funky. -- To view, visit http://gerrit.cloudera.org:8080/6495 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
