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.

Sign up for GitHub

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

Jump to bottom

expression: fix type infer for decimal property in count agg #17368

Merged
merged 17 commits into from
Jun 5, 2020
Merged

expression: fix type infer for decimal property in count agg #17368

merged 17 commits into from
Jun 5, 2020

Conversation

lysu
Copy link
Contributor

@lysu lysu commented May 22, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #17367

Problem Summary:

type infer for decimal property in count agg in inaccurate.

so it will make some GUI client output some stranger results like this:

image

What is changed and how it works?

What's Changed:

make count agg's decimal property always be 0(because count result always be a interger)

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (test with GUI)

Side effects

  • n/a

Release note

  • Fix type infer for decimal property in count agg

This change is Reviewable

@lysu lysu requested a review from a team as a code owner May 22, 2020 11:37
@ghost ghost requested review from wshwsh12 and removed request for a team May 22, 2020 11:37
@pingcap pingcap deleted a comment from sre-bot May 22, 2020
@pingcap pingcap deleted a comment from sre-bot May 22, 2020
@lysu
Copy link
Contributor Author

lysu commented May 22, 2020

/run-all-tests

@lysu
Copy link
Contributor Author

lysu commented May 22, 2020

/rebuild

@lysu lysu requested a review from XuHuaiyu May 22, 2020 11:42
@lysu lysu changed the title expression: Fix type infer for decimal property in count agg expression: fix type infer for decimal property in count agg May 22, 2020
@codecov
Copy link

codecov bot commented May 22, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #17368   +/-   ##
===========================================
  Coverage   79.5044%   79.5044%           
===========================================
  Files           524        524           
  Lines        141445     141445           
===========================================
  Hits         112455     112455           
  Misses        19928      19928           
  Partials       9062       9062

@lysu lysu requested review from SunRunAway and qw4990 and removed request for XuHuaiyu May 25, 2020 03:25
@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2020

@SunRunAway, @qw4990, @wshwsh12, PTAL.

SunRunAway
SunRunAway reviewed May 27, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label May 27, 2020
wshwsh12
wshwsh12 approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 28, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 28, 2020

Your auto merge job has been accepted, waiting for:

  • 17418
  • 17395
  • 17359
  • 17461

@SunRunAway
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

@lysu merge failed.

@lysu
Copy link
Contributor Author

lysu commented Jun 5, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

Sorry @lysu, you don't have permission to trigger auto merge event on this branch. You are not a committer for this part

@lysu
Copy link
Contributor Author

lysu commented Jun 5, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

Your auto merge job has been accepted, waiting for:

  • 17683

@lysu
Copy link
Contributor Author

lysu commented Jun 5, 2020

/run-common-test

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

@lysu merge failed.

@lysu lysu merged commit 583956d into pingcap:master Jun 5, 2020
@lysu lysu deleted the dev-fix-count-type-decial-infer branch June 5, 2020 04:43
@lysu lysu added this to the v4.0.2 milestone Jun 5, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 5, 2020
@lysu @sre-bot
cherry pick pingcap#17368 to release-3.0
dbdf64d 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Jun 5, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

cherry pick to release-3.0 in PR #17702

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 5, 2020
@lysu @sre-bot
cherry pick pingcap#17368 to release-3.1
2f5b4e5 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Jun 5, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

cherry pick to release-3.1 in PR #17703

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 5, 2020
@lysu @sre-bot
cherry pick pingcap#17368 to release-4.0
51230c4 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Jun 5, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jun 5, 2020

cherry pick to release-4.0 in PR #17704

SunRunAway pushed a commit that referenced this pull request Jun 5, 2020
@sre-bot
expression: fix type infer for decimal property in count agg (#17368)
37455e6
sre-bot added a commit that referenced this pull request Jun 7, 2020
@sre-bot @lysu
expression: fix type infer for decimal property in count agg (#17368) (…
5f52591 
…#17702)

Signed-off-by: sre-bot <sre-bot@pingcap.com>

Co-authored-by: lysu <sulifx@gmail.com>
sre-bot added a commit that referenced this pull request Jun 7, 2020
@sre-bot @lysu
expression: fix type infer for decimal property in count agg (#17368) (…
86211f0 
…#17703)

Signed-off-by: sre-bot <sre-bot@pingcap.com>

Co-authored-by: lysu <sulifx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Milestone
v4.0.2
Development

Successfully merging this pull request may close these issues.

count agg's type infer for decimal property is inaccurate
6 participants
@lysu @sre-bot @jackysp @SunRunAway @zz-jason @wshwsh12