Skip to content

planner: fix push down distinct when need to inject projection#15997

Merged
eurekaka merged 1 commit into
pingcap:masterfrom
SunRunAway:issue14623-pushdown-distinct
Apr 2, 2020
Merged

planner: fix push down distinct when need to inject projection#15997
eurekaka merged 1 commit into
pingcap:masterfrom
SunRunAway:issue14623-pushdown-distinct

Conversation

@SunRunAway
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway commented Apr 2, 2020

What problem does this PR solve?

Issue Number: close #15999

Problem Summary:
If aggregation function is avg(distinct) or sum(distinct) when tidb_opt_distinct_agg_push_down is set to 1, Planner needs to inject a projection to cast cop data from type int to type decimal. So a property of finalAgg is necessary or it will panic.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

Check List

Tests

  • Unit test

Side effects

  • None

@SunRunAway SunRunAway requested a review from a team as a code owner April 2, 2020 03:07
@ghost ghost requested review from eurekaka and removed request for a team April 2, 2020 03:08
@SunRunAway SunRunAway force-pushed the issue14623-pushdown-distinct branch from 12a494b to f9cf607 Compare April 2, 2020 03:11
@SunRunAway SunRunAway requested review from winoros and zz-jason April 2, 2020 03:15
@SunRunAway SunRunAway added the sig/planner SIG: Planner label Apr 2, 2020
@SunRunAway SunRunAway changed the title executor: fix push down distinct when need to inject projection planner: fix push down distinct when need to inject projection Apr 2, 2020
@SunRunAway SunRunAway added needs-cherry-pick-4.0 priority/release-blocker This issue blocks a release. Please solve it ASAP. type/bugfix This PR fixes a bug. priority/P1 The issue has P1 priority. labels Apr 2, 2020
Copy link
Copy Markdown
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 2, 2020
@SunRunAway
Copy link
Copy Markdown
Contributor Author

/run-all-tests

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 2, 2020

Codecov Report

Merging #15997 into master will increase coverage by 0.0506%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #15997        +/-   ##
================================================
+ Coverage   80.4506%   80.5012%   +0.0506%     
================================================
  Files           505        505                
  Lines        135897     135291       -606     
================================================
- Hits         109330     108911       -419     
+ Misses        18062      17875       -187     
  Partials       8505       8505

Copy link
Copy Markdown
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Apr 2, 2020

cherry pick to release-4.0 in PR #16000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/P1 The issue has P1 priority. priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/planner SIG: Planner status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

avg(distinct) panicked when tidb_opt_distinct_agg_push_down is enabled

4 participants