Lars Volker has posted comments on this change. Change subject: IMPALA-5144: Remove sortby() hint ......................................................................
Patch Set 2: (3 comments) Thank you for having a look. I added a test in PS3, and inline comments pointing to the remaining tests you asked for. http://gerrit.cloudera.org:8080/#/c/6885/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: PS2, Line 1792: > Do we have any tests that use shuffle and clustered? If not, maybe we shoul Tests for shuffle and clustered are above this removed block, starting in Line 1726. The tests here have been added for the sortby() hint in particular, and were also only executed for non-legacy syntax. The tests above run for all hint syntaxes. http://gerrit.cloudera.org:8080/#/c/6885/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: PS2, Line 508: > Since you're at it, maybe add a simply test with clustered as well. Didn't Good idea, done. http://gerrit.cloudera.org:8080/#/c/6885/2/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test: PS2, Line 328: > Same comment as before. Just make sure we have some coverage for shuffle + They are covered in multiple combinations in insert.test. -- To view, visit http://gerrit.cloudera.org:8080/6885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I83e1cd6fa7039035973676322deefbce00d3f594 Gerrit-PatchSet: 2 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-HasComments: Yes
