Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs ...................................................................... Patch Set 4: (3 comments) Looking closely into the tests now. http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@889 PS3, Line 889: // grouping exprs in the second phase which is not required when omitting the distinct > What do you mean by separating them? I suppose we always shuffle if there a Nvm, current code is ok. http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@931 PS3, Line 931: // phase-1 merge step. > L929 adds phase-2 agg. Partition being compatible && no shuffle by distinct Got it, for some reason I missed L929. http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@952 PS3, Line 952: // step (which is where it should be) > We don't need "fragments.add(firstMergeFragment)" if the partition is compa Makes sense. Let's the two paths more uniform with: if (!shuffleDistinctExpr) return firstMergeFragment; fragments.add(firstMergeFragment); -- To view, visit http://gerrit.cloudera.org:8080/9949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc Gerrit-Change-Number: 9949 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Apr 2018 22:37:03 +0000 Gerrit-HasComments: Yes
