executor: check for null values when comparing different groups during streamAgg #15742

Reminiscent · 2020-03-26T12:38:59Z

What problem does this PR solve?

Close #15690
Close #15691
Close #15695
Close #15696
In the master version, we forget to check the null value when we compare the different groups during StreamAgg.

What is changed and how it works?

Add checks for null values when comparing different groups during StreamAgg.

Check List

Tests

Unit test
Integration test

…g streamAgg

…5696

codecov · 2020-03-26T12:45:15Z

Codecov Report

Merging #15742 into master will decrease coverage by 0.0521%.
The diff coverage is 76.5957%.

@@               Coverage Diff                @@
##             master     #15742        +/-   ##
================================================
- Coverage   80.4669%   80.4148%   -0.0522%     
================================================
  Files           504        504                
  Lines        134649     134617        -32     
================================================
- Hits         108348     108252        -96     
- Misses        17831      17883        +52     
- Partials       8470       8482        +12

qw4990 · 2020-03-26T13:13:20Z

Do we need to cherry-pick this PR to release-4.0?

qw4990 · 2020-03-26T13:21:56Z

executor/aggregate_test.go

+	tk := testkit.NewTestKitWithInit(c, s.store)
+	tk.MustExec(`drop table if exists t;`)
+	tk.MustExec(`create table t(a int);`)
+	tk.MustExec(`insert into t values(null),(null);`)
+	tk.MustExec(`insert into t values(0),(2),(2),(4),(8);`)
+	tk.Se.GetSessionVars().MaxChunkSize = 2
+	tk.MustQuery(`select /*+ stream_agg() */ distinct * from t;`).Check(testkit.Rows("<nil>", "0", "2", "4", "8"))


Almost LGTM, but please add more tests to cover more types for safety.

XuHuaiyu

Addressing the comment makes LGTM

SunRunAway

Could you add some test cases for splitIntoGroups like others https://github.com/search?q=splitIntoGroups+repo%3Apingcap%2Ftidb+filename%3A_test.go&type=Code&ref=advsearch&l=&l=?

…5696

Reminiscent · 2020-03-27T07:28:17Z

/run-all-tests

Reminiscent · 2020-03-27T07:35:18Z

/run-unit-test

Reminiscent · 2020-03-27T07:36:04Z

@SunRunAway @qw4990 @XuHuaiyu Update. PTAL

Reminiscent · 2020-03-27T07:46:38Z

Could you add some test cases for splitIntoGroups like others https://github.com/search?q=splitIntoGroups+repo%3Apingcap%2Ftidb+filename%3A_test.go&type=Code&ref=advsearch&l=&l=?

tidb/executor/executor_required_rows_test.go

Lines 814 to 816 in 7746fbd

    
           flag, err := groupChecker.splitIntoGroups(inputChk) 
        
           c.Assert(err, IsNil) 
        
           c.Assert(flag, Equals, testCase.expectedFlag[i])

Here can check the result of splitIntoGroups .

XuHuaiyu

LGTM

SunRunAway

Rest LGTM

executor/aggregate_test.go

…5696

SunRunAway

LGTM

sre-bot · 2020-03-27T08:38:44Z

/run-all-tests

…g streamAgg (pingcap#15742)

sre-bot · 2020-03-27T08:43:23Z

cherry pick to release-4.0 in PR #15777

…g streamAgg (#15742) (#15777)

fzhedu · 2020-03-28T08:50:41Z

executor/aggregate.go

+	if firstRowIsNull {
+		firstRowDatum.SetNull()
+	}
+	if lastRowIsNull {
+		lastRowDatum.SetNull()
+	}


move this behand switch eType, and reduce the if to check null in the switch.

Reminiscent added 2 commits March 26, 2020 20:26

executor: check for null values when comparing different groups durin…

1fd1552

…g streamAgg

Merge branch 'master' of https://github.com/pingcap/tidb into issue-1…

2891ef1

…5696

Reminiscent added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Mar 26, 2020

Reminiscent requested review from fzhedu and XuHuaiyu March 26, 2020 12:38

Reminiscent requested a review from a team as a code owner March 26, 2020 12:38

ghost removed their request for review March 26, 2020 12:39

Reminiscent added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Mar 26, 2020

XuHuaiyu requested a review from qw4990 March 26, 2020 12:45

qw4990 reviewed Mar 26, 2020

View reviewed changes

XuHuaiyu added the needs-cherry-pick-4.0 label Mar 26, 2020

XuHuaiyu reviewed Mar 26, 2020

View reviewed changes

SunRunAway reviewed Mar 26, 2020

View reviewed changes

Reminiscent added 3 commits March 27, 2020 14:42

add more tests

5371cee

Merge branch 'master' of https://github.com/pingcap/tidb into issue-1…

7746fbd

…5696

clean the code

a11b2ef

XuHuaiyu requested review from qw4990 and SunRunAway March 27, 2020 07:26

clean the test code

e23ea12

Reminiscent requested a review from XuHuaiyu March 27, 2020 07:35

XuHuaiyu reviewed Mar 27, 2020

View reviewed changes

SunRunAway reviewed Mar 27, 2020

View reviewed changes

executor/aggregate_test.go Show resolved Hide resolved

address comment

bf95bf7

Reminiscent added 2 commits March 27, 2020 16:25

Merge branch 'master' of https://github.com/pingcap/tidb into issue-1…

b922def

…5696

address comment

0dca4bc

SunRunAway approved these changes Mar 27, 2020

View reviewed changes

SunRunAway added the status/can-merge Indicates a PR has been approved by a committer. label Mar 27, 2020

sre-bot merged commit 0d10f91 into pingcap:master Mar 27, 2020

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 27, 2020

executor: check for null values when comparing different groups durin…

f8dcbb5

…g streamAgg (pingcap#15742)

sre-bot mentioned this pull request Mar 27, 2020

executor: check for null values when comparing different groups during streamAgg (#15742) #15777

Merged

ngaut pushed a commit that referenced this pull request Mar 28, 2020

executor: check for null values when comparing different groups durin…

c07728a

…g streamAgg (#15742) (#15777)

fzhedu reviewed Mar 28, 2020

View reviewed changes

Reminiscent deleted the issue-15696 branch August 5, 2021 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: check for null values when comparing different groups during streamAgg #15742

executor: check for null values when comparing different groups during streamAgg #15742

Reminiscent commented Mar 26, 2020 •

edited by XuHuaiyu

Loading

codecov bot commented Mar 26, 2020 •

edited

Loading

qw4990 commented Mar 26, 2020

qw4990 Mar 26, 2020

XuHuaiyu left a comment

SunRunAway left a comment •

edited

Loading

Reminiscent commented Mar 27, 2020

Reminiscent commented Mar 27, 2020 •

edited

Loading

Reminiscent commented Mar 27, 2020

Reminiscent commented Mar 27, 2020

XuHuaiyu left a comment

SunRunAway left a comment

SunRunAway left a comment

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

fzhedu Mar 28, 2020

executor: check for null values when comparing different groups during streamAgg #15742

executor: check for null values when comparing different groups during streamAgg #15742

Conversation

Reminiscent commented Mar 26, 2020 • edited by XuHuaiyu Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Mar 26, 2020 • edited Loading

Codecov Report

qw4990 commented Mar 26, 2020

qw4990 Mar 26, 2020

Choose a reason for hiding this comment

XuHuaiyu left a comment

Choose a reason for hiding this comment

SunRunAway left a comment • edited Loading

Choose a reason for hiding this comment

Reminiscent commented Mar 27, 2020

Reminiscent commented Mar 27, 2020 • edited Loading

Reminiscent commented Mar 27, 2020

Reminiscent commented Mar 27, 2020

XuHuaiyu left a comment

Choose a reason for hiding this comment

SunRunAway left a comment

Choose a reason for hiding this comment

SunRunAway left a comment

Choose a reason for hiding this comment

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

fzhedu Mar 28, 2020

Choose a reason for hiding this comment

Reminiscent commented Mar 26, 2020 •

edited by XuHuaiyu

Loading

codecov bot commented Mar 26, 2020 •

edited

Loading

SunRunAway left a comment •

edited

Loading

Reminiscent commented Mar 27, 2020 •

edited

Loading