You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tianyi Wang (Code Review)" <ge...@cloudera.org> on 2018/04/07 00:49:49 UTC
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9949
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-on-distinct.test
7 files changed, 856 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/1
--
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: newchange
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/distinct.test
M tests/query_test/test_aggregation.py
9 files changed, 454 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/6
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/distinct.test
M tests/query_test/test_aggregation.py
9 files changed, 371 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/5
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9949/3/be/src/service/query-options.h
File be/src/service/query-options.h:
http://gerrit.cloudera.org:8080/#/c/9949/3/be/src/service/query-options.h@44
PS3, Line 44: TImpalaQueryOptions::SHUFFLE_DISTINCT_EXPR + 1);\
Let's call this SHUFFLE_DISTINCT_EXPRS because there could be several such exprs
http://gerrit.cloudera.org:8080/#/c/9949/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/9949/3/common/thrift/ImpalaInternalService.thrift@276
PS3, Line 276: // When a query has both grouping exprs and a distinct expr, impala can shuffle by
* both grouping and distinct exprs
* Impala can optionally include the distinct exprs in the hash exchange of the first aggregation phase to spread the data among more nodes. However, this plan requires another hash exchange on the grouping exprs in the second phase which is not required when when omitting the distinct exprs in the first phase. Shuffling by both is better if the grouping exprs have a low NDVs.
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@886
PS3, Line 886: // When a query has both grouping exprs and a distinct expr, impala can shuffle by
same comments as in .thrift
http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@889
PS3, Line 889: boolean shuffleDistinctExpr = ctx_.getQueryOptions().shuffle_distinct_expr ||
shuffleDistinctExprs
I think it's cleaner to separate the value of the query option from hasGroupingExprs
http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@931
PS3, Line 931: if (!shuffleDistinctExpr) return firstMergeFragment;
I don't understand why this is correct. If the first phase fragment is suitably partitioned then we still need to do the second phase. Where do we add the second phase aggregation if we return here?
Is this because shuffleDistinctExpr conflates whether there are grouping exprs or not and what the query option is set to?
http://gerrit.cloudera.org:8080/#/c/9949/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@952
PS3, Line 952: if (shuffleDistinctExpr) {
Move this before L959 for better readability. Also you can just:
if (!shuffleDistinctExpr) return firstFragment;
http://gerrit.cloudera.org:8080/#/c/9949/3/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
File testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test:
http://gerrit.cloudera.org:8080/#/c/9949/3/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test@1
PS3, Line 1: # distinct agg with grouping over a union
Let's try to distill minimal tests that are needed here.
* distinct with and without grouping
* first phase fragment is already suitably partitioned or not
Looks like we have a bunch of tests here with unnecessary stuff like unions, etc.
I'll spend some time looking over these and the existing tests, so we can come up with a lean and effective test suite.
--
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: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 19:16:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
7 files changed, 856 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/3
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:37:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 6:
doesn't look like this change included the fix for impala_6215
--
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: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 17:02:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2287/
--
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: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 21:13:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
7 files changed, 173 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/4
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7: Verified+1
--
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: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 22:01:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2298/
--
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: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 18:15:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2287/
--
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: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 01:39:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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 7: Code-Review+2
--
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: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 18:14:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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:
(2 comments)
We have no end-to-end coverage with the option set to false. I think we need to expand test_aggregation.py to cover both variants.
http://gerrit.cloudera.org:8080/#/c/9949/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:
http://gerrit.cloudera.org:8080/#/c/9949/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@79
PS4, Line 79: TQueryOptions options = defaultQueryOptions();
Let's run both variants in this .test file. You can use ---- a QUERYOPTIONS section to set the value of the query option for each QUERY.
See runtime-filter-query-options.test for an example use
http://gerrit.cloudera.org:8080/#/c/9949/4/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
File testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test:
http://gerrit.cloudera.org:8080/#/c/9949/4/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test@6
PS4, Line 6: |/home/twang/projects/impala/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:[892,58]
clean
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:51:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9949/5/be/src/service/query-options.h
File be/src/service/query-options.h:
http://gerrit.cloudera.org:8080/#/c/9949/5/be/src/service/query-options.h@132
PS5, Line 132: QUERY_OPT_FN(shuffle_distinct_exprs, SHUFFLE_DISTINCT_EXPRS, TQueryOptionLevel::REGULAR)\
This seems similarly ADVANCED as, e.g., DEFAULT_JOIN_DISTRIBUTION_MODE so let's move to ADVANCED.
http://gerrit.cloudera.org:8080/#/c/9949/5/be/src/service/query-options.cc
File be/src/service/query-options.cc:
http://gerrit.cloudera.org:8080/#/c/9949/5/be/src/service/query-options.cc@635
PS5, Line 635: case TImpalaQueryOptions::SHUFFLE_DISTINCT_EXPRS: {
I just realized we should also update the query-options-test.cc
http://gerrit.cloudera.org:8080/#/c/9949/5/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:
http://gerrit.cloudera.org:8080/#/c/9949/5/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@78
PS5, Line 78: public void testNoShuffleOnDistinct() {
testShuffleByDistinctExprs()
http://gerrit.cloudera.org:8080/#/c/9949/5/testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test
File testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test:
http://gerrit.cloudera.org:8080/#/c/9949/5/testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test@2
PS5, Line 2: ---- QUERY
Is the formatting of these tests correct? I believe we don't recognize "---- QUERY in planner tests" Or perhaps this works by chance? Other tests don't have this better to be consistent.
http://gerrit.cloudera.org:8080/#/c/9949/5/testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test@217
PS5, Line 217: select count(distinct a.int_col) from functional.alltypes a inner join [shuffle]
Add one more test where the input is partitioned by year, int_col (compatible with the desired partitioning with distinct exprs)
http://gerrit.cloudera.org:8080/#/c/9949/5/tests/query_test/test_aggregation.py
File tests/query_test/test_aggregation.py:
http://gerrit.cloudera.org:8080/#/c/9949/5/tests/query_test/test_aggregation.py@336
PS5, Line 336: class TestDistinctAggregationQueries(ImpalaTestSuite):
TestDistinctAggregation
http://gerrit.cloudera.org:8080/#/c/9949/5/tests/query_test/test_aggregation.py@353
PS5, Line 353: if cls.exploration_strategy() == 'core':
Aren't we adding all formats in L345? I think we need to add a constraint here and not a new dimension.
--
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: 5
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 03:49:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang 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:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9949/3/be/src/service/query-options.h
File be/src/service/query-options.h:
http://gerrit.cloudera.org:8080/#/c/9949/3/be/src/service/query-options.h@44
PS3, Line 44: TImpalaQueryOptions::SHUFFLE_DISTINCT_EXPRS + 1);\
> Let's call this SHUFFLE_DISTINCT_EXPRS because there could be several such
Done
http://gerrit.cloudera.org:8080/#/c/9949/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/9949/3/common/thrift/ImpalaInternalService.thrift@276
PS3, Line 276: // When a query has both grouping and distinct exprs, impala can optionally include the
> * both grouping and distinct exprs
Done
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@886
PS3, Line 886: // When a query has both grouping and distinct exprs, impala can optionally include
> same comments as in .thrift
Done
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
> shuffleDistinctExprs
What do you mean by separating them? I suppose we always shuffle if there aren't grouping exprs.
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.
> I don't understand why this is correct. If the first phase fragment is suit
L929 adds phase-2 agg. Partition being compatible && no shuffle by distinct exprs => the child is partitioned by grouping exprs. We only need phase-1 agg and phase-2 agg without any merge agg or exchange.
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)
> Move this before L959 for better readability. Also you can just:
We don't need "fragments.add(firstMergeFragment)" if the partition is compatible. How to move it out?
http://gerrit.cloudera.org:8080/#/c/9949/3/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
File testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test:
http://gerrit.cloudera.org:8080/#/c/9949/3/testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test@1
PS3, Line 1: # Distinct agg without a grouping expr
> Let's try to distill minimal tests that are needed here.
Done
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:19:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/distinct.test
M tests/query_test/test_aggregation.py
9 files changed, 454 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/7
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+2
--
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: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 20:54:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Reviewed-on: http://gerrit.cloudera.org:8080/9949
Reviewed-by: Tianyi Wang <tw...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/shuffle-by-distinct-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/distinct.test
M tests/query_test/test_aggregation.py
9 files changed, 454 insertions(+), 24 deletions(-)
Approvals:
Tianyi Wang: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6822: Add a query option to control shuffling by distinct exprs
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9949 )
Change subject: IMPALA-6822: Add a query option to control shuffling by distinct exprs
......................................................................
IMPALA-6822: Add a query option to control shuffling by distinct exprs
IMPALA-4794 changed the distinct aggregation behavior to shuffling by
both grouping exprs and the distinct expr. It's slower in queries
where the NDVs of grouping exprs are high and data are uniformly
distributed among groups. This patch adds a query option controlling
this behavior, letting users switch to the old plan.
Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/no-shuffle-by-distinct.test
7 files changed, 856 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/9949/2
--
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: newpatchset
Gerrit-Change-Id: Icb4b4576fb29edd62cf4b4ba0719c0e0a2a5a8dc
Gerrit-Change-Number: 9949
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>