Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: split avg to count and sum for TableReader cop task #11926

Merged
merged 4 commits into from Sep 2, 2019

Conversation

@lzmhhh123
Copy link
Member

commented Aug 29, 2019

What problem does this PR solve?

Because the calculation of avg in the coprocessor is split as sum and count, and it also returns two columns. We can split avg in the planner. Now it's only effective for cop agg in TableReader.

A sub PR for support the coprocessor in tiflash.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • None
@codecov

This comment has been minimized.

Copy link

commented Aug 29, 2019

Codecov Report

Merging #11926 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11926   +/-   ##
===========================================
  Coverage   81.4033%   81.4033%           
===========================================
  Files           444        444           
  Lines         95560      95560           
===========================================
  Hits          77789      77789           
  Misses        12291      12291           
  Partials       5480       5480
@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/run-all-tests

@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/run-integration-common-test

1 similar comment
@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/run-integration-common-test

@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@lzmhhh123 Does it affect the execution logic on TiKV Coprocessor?

@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@lzmhhh123 Does it affect the execution logic on TiKV Coprocessor?

I'm checking this situation.

@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/run-all-tests

@@ -864,7 +864,7 @@ func (s *testPlanSuite) TestDAGPlanBuilderAgg(c *C) {
// Test agg + table.
{
sql: "select sum(a), avg(b + c) from t group by d",
best: "TableReader(Table(t)->HashAgg)->HashAgg",
best: "TableReader(Table(t))->Projection->HashAgg",

This comment has been minimized.

Copy link
@winoros

winoros Aug 29, 2019

Member

Why this one changed?

This comment has been minimized.

Copy link
@lzmhhh123

lzmhhh123 Aug 29, 2019

Author Member

I felt confused too. So I'm investigating the reason now.

This comment has been minimized.

Copy link
@eurekaka

eurekaka Aug 29, 2019

Contributor

I guess it should be caused by the fact that the number of AggFuncs increases, so the cost of Aggregation is changed, and planner decides not to push this Agg down to coprocessor.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 30, 2019

Member

According to #11926 (comment), seems we should modify the cost model to decrease the impact of number of aggregate functions to make this aggregation can be pushed to tikv?

This comment has been minimized.

Copy link
@lzmhhh123

lzmhhh123 Aug 30, 2019

Author Member

We can temporarily not to modify the cost model. That's another task of cost model improvement with the consideration of aggregation type.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 30, 2019

Member

OK, How about filing an issue about this improvement?

This comment has been minimized.

Copy link
@lzmhhh123

lzmhhh123 Aug 30, 2019

Author Member

Okay.

@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@lzmhhh123 Does it affect the execution logic on TiKV Coprocessor?

I'm checking this situation.

It passed the integration-common-test at least.

@lzmhhh123 lzmhhh123 requested a review from zz-jason Aug 29, 2019

@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@lzmhhh123 Does it affect the execution logic on TiKV Coprocessor?

I'm checking this situation.

It passed the integration-common-test at least.

Yeah, does it mean that the execution instruction in TiKV is not changed? I'm afraid that this pr would impact the end-to-end performance of a SQL.

@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@lzmhhh123 Does it affect the execution logic on TiKV Coprocessor?

I'm checking this situation.

It passed the integration-common-test at least.

Yeah, does it mean that the execution instruction in TiKV is not changed? I'm afraid that this pr would impact the end-to-end performance of a SQL.

Only a little. Because TiKV executes avg as sum and count.

@@ -864,7 +864,7 @@ func (s *testPlanSuite) TestDAGPlanBuilderAgg(c *C) {
// Test agg + table.
{
sql: "select sum(a), avg(b + c) from t group by d",
best: "TableReader(Table(t)->HashAgg)->HashAgg",
best: "TableReader(Table(t))->Projection->HashAgg",

This comment has been minimized.

Copy link
@eurekaka

eurekaka Aug 29, 2019

Contributor

I guess it should be caused by the fact that the number of AggFuncs increases, so the cost of Aggregation is changed, and planner decides not to push this Agg down to coprocessor.

planner/core/task.go Show resolved Hide resolved
planner/core/task.go Show resolved Hide resolved
@eurekaka
Copy link
Contributor

left a comment

LGTM

@zz-jason
Copy link
Member

left a comment

LGTM

@sre-bot

This comment has been minimized.

Copy link

commented Sep 2, 2019

/run-all-tests

@sre-bot sre-bot merged commit b239f2f into pingcap:master Sep 2, 2019

13 checks passed

idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@lzmhhh123 lzmhhh123 deleted the lzmhhh123:dev/split_avg_to_countSum branch Sep 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.