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, planner: fix decimal results for aggregate functions #20017

Merged
merged 20 commits into from
Nov 11, 2020
Merged

expression, planner: fix decimal results for aggregate functions #20017

merged 20 commits into from
Nov 11, 2020

Conversation

dyzsr
Copy link
Contributor

@dyzsr dyzsr commented Sep 15, 2020 •
edited by ti-srebot
Loading

What problem does this PR solve?

Issue Number: close #19426, fix PR #19592

Problem Summary:

Execute

create table t(a int primary key, b int);
insert into t values (1, 11), (4, 44), (2, 22), (3, 33);

Obtained

mysql> select sum(case when a <= 0 or a > 1000 then 0.0 else b end) from t;
+-------------------------------------------------------+
| sum(case when a <= 0 or a > 1000 then 0.0 else b end) |
+-------------------------------------------------------+
|                                                   110 |
+-------------------------------------------------------+
1 row in set (0.01 sec)

mysql> select group_concat(case when a <= 0 or a > 1000 then 0.0 else b end) from t;
+----------------------------------------------------------------+
| group_concat(case when a <= 0 or a > 1000 then 0.0 else b end) |
+----------------------------------------------------------------+
| 11,22,33,44                                                    |
+----------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select distinct case when a <= 0 or a > 1000 then 0.0 else b end from t;
+--------------------------------------------------+
| case when a <= 0 or a > 1000 then 0.0 else b end |
+--------------------------------------------------+
|                                               11 |
|                                               22 |
|                                               33 |
|                                               44 |
+--------------------------------------------------+
4 rows in set (0.00 sec)

Expect

mysql> select sum(case when a <= 0 or a > 1000 then 0.0 else b end) from t;
+-------------------------------------------------------+
| sum(case when a <= 0 or a > 1000 then 0.0 else b end) |
+-------------------------------------------------------+
|                                                 110.0 |
+-------------------------------------------------------+
1 row in set (0.00 sec)

mysql>  select group_concat(case when a <= 0 or a > 1000 then 0.0 else b end) from t;
+----------------------------------------------------------------+
| group_concat(case when a <= 0 or a > 1000 then 0.0 else b end) |
+----------------------------------------------------------------+
| 11.0,22.0,33.0,44.0                                            |
+----------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select distinct case when a <= 0 or a > 1000 then 0.0 else b end from t;
+--------------------------------------------------+
| case when a <= 0 or a > 1000 then 0.0 else b end |
+--------------------------------------------------+
|                                             11.0 |
|                                             22.0 |
|                                             33.0 |
|                                             44.0 |
+--------------------------------------------------+
4 rows in set (0.00 sec)

What is changed and how it works?

  • wrap cast as decimal for arguments of group_concat to preserve fraction part digits of the results.
  • round the decimal final result in AppendFinalResult2Chunk to the argument's precision.

Related changes

  • Need to cherry-pick to the release branch 3.0, 4.0

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • fix the fraction part of decimal results for aggregate functions

@dyzsr dyzsr requested review from a team as code owners September 15, 2020 13:24
@dyzsr dyzsr requested review from qw4990 and SunRunAway and removed request for a team September 15, 2020 13:24
@github-actions github-actions bot added the sig/execution SIG execution label Sep 15, 2020
@dyzsr dyzsr marked this pull request as draft September 15, 2020 13:37
@dyzsr dyzsr marked this pull request as ready for review September 15, 2020 15:24
@dyzsr
Copy link
Contributor Author

dyzsr commented Sep 15, 2020

/run-all-tests

@dyzsr
Copy link
Contributor Author

dyzsr commented Sep 16, 2020

/run-all-tests

@dyzsr
Copy link
Contributor Author

dyzsr commented Sep 16, 2020

PTAL @wjhuang2016

@dyzsr dyzsr changed the title expression, planner: wrap cast as decimal for aggregate function arguments expression, planner: fix decimal results for aggregate functions Sep 16, 2020
SunRunAway
SunRunAway reviewed Sep 18, 2020
View reviewed changes
executor/aggfuncs/func_first_row.go Outdated
@@ -16,6 +16,9 @@ package aggfuncs
import (
"unsafe"

"github.com/cznic/mathutil"
"github.com/pingcap/parser/mysql"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the empty line

expression/aggregation/base_func.go
func (a *baseFuncDesc) WrapCastAsDecimalForAggArgs(ctx sessionctx.Context) {
if _, ok := needCastAsDecimalAggFuncs[a.Name]; ok {
for i := range a.Args {
if tp := a.Args[i].GetType(); tp.Tp == mysql.TypeNewDecimal {
Copy link
Contributor

Choose a reason for hiding this comment

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

The last argument of group_cancat its separator, is it ok?

SunRunAway
SunRunAway reviewed Sep 18, 2020
View reviewed changes
expression/aggregation/base_func.go Outdated
func (a *baseFuncDesc) WrapCastAsDecimalForAggArgs(ctx sessionctx.Context) {
if a.Name == ast.AggFuncGroupConcat {
if tp := a.Args[0].GetType(); tp.Tp == mysql.TypeNewDecimal {
a.Args[0] = expression.BuildCastFunction(ctx, a.Args[0], tp)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the second argument needs to be cast?

select group_concat(a, case when a <= 2 or a > 1000 then 0.0 else b end) from t;

@qw4990
Copy link
Contributor

qw4990 commented Nov 4, 2020

/run-all-tests

qw4990
qw4990 approved these changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 4, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 4, 2020
@dyzsr
Copy link
Contributor Author

dyzsr commented Nov 11, 2020

/run-all-tests

@lzmhhh123
Copy link
Contributor

/run-common-test tidb-test=pr/1100
/run-integration-common-test tidb-test=pr/1100

@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 11, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@lzmhhh123
Copy link
Contributor

/run-all-tests

@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@lzmhhh123 lzmhhh123 merged commit a3facd0 into pingcap:master Nov 11, 2020
@lzmhhh123
Copy link
Contributor

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 11, 2020
@dyzsr @ti-srebot
cherry pick pingcap#20017 to release-4.0
acdb935 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Nov 11, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20984

jebter pushed a commit that referenced this pull request Dec 1, 2020
@ti-srebot @dyzsr
@qw4990 @winoros
expression, planner: fix decimal results for aggregate functions (#20017
8a2b35f 
) (#20984)

* cherry pick #20017 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* resolve conflicts

Co-authored-by: dy <34701401+dyzsr@users.noreply.github.com>
Co-authored-by: dongyan <dybest@icloud.com>
Co-authored-by: Yuanjia Zhang <zhangyuanjia@pingcap.com>
Co-authored-by: Yiding Cui <winoros@gmail.com>
@fzhedu fzhedu mentioned this pull request Nov 26, 2021
Merged
12 tasks
This was referenced Dec 3, 2021
Merged
Merged
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winoros winoros winoros left review comments

@SunRunAway SunRunAway SunRunAway left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@qw4990 qw4990 qw4990 approved these changes

Assignees
No one assigned
Labels
challenge-program sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

select sum(case when a <= 0 or a > 1000 then 0.0 else b end) from t returns wrong decimal
7 participants
@dyzsr @qw4990 @lzmhhh123 @ti-srebot @SunRunAway @winoros @wjhuang2016