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

executor: use deep copy for maxMin4JSON #15242

Merged
merged 5 commits into from
Mar 11, 2020
Merged

executor: use deep copy for maxMin4JSON #15242

merged 5 commits into from
Mar 11, 2020

Conversation

fzhedu
Copy link
Contributor

@fzhedu fzhedu commented Mar 9, 2020 •
edited
Loading

What problem does this PR solve?

fix #15278

What is changed and how it works?

change shallow copy to deep copy

Check List

Tests

  • Manual test (add detailed scripts or steps below)

use large dataset to reproduce this problem.

Code changes

  • No

Side effects

  • No

Related changes

  • Need to cherry-pick to the release branch

Release note

  • No

@fzhedu fzhedu requested a review from a team as a code owner March 9, 2020 13:22
@ghost ghost requested review from qw4990 and wshwsh12 and removed request for a team March 9, 2020 13:22
@fzhedu fzhedu added sig/execution SIG execution type/bugfix This PR fixes a bug. labels Mar 9, 2020
wshwsh12
wshwsh12 reviewed Mar 10, 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 the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2020
SunRunAway
SunRunAway reviewed Mar 10, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment •
edited
Loading

Choose a reason for hiding this comment

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

solve the bug results from using shallow copy in maxMin4JSON

Could you create an issue describing the detail of the bug for this PR?

@qw4990 qw4990 removed their request for review March 10, 2020 07:48
@fzhedu fzhedu requested a review from SunRunAway March 10, 2020 13:39
@fzhedu
Copy link
Contributor Author

fzhedu commented Mar 10, 2020 •
edited
Loading

solve the bug results from using shallow copy in maxMin4JSON

Could you create an issue describing the detail of the bug for this PR?

It is hard to reproduce this issue using a small data set. So, I cannot.

@zz-jason
Copy link
Member

solve the bug results from using shallow copy in maxMin4JSON

Could you create an issue describing the detail of the bug for this PR?

It is hard to reproduce this issue using a small data set. So, I cannot.

  1. you can create a large data set to reproduce the problem
  2. you can use a small chunk size to reproduce the problem

@codecov
Copy link

codecov bot commented Mar 11, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #15242   +/-   ##
===========================================
  Coverage   80.3683%   80.3683%           
===========================================
  Files           503        503           
  Lines        133061     133061           
===========================================
  Hits         106939     106939           
  Misses        17712      17712           
  Partials       8410       8410

@fzhedu
Copy link
Contributor Author

fzhedu commented Mar 11, 2020 •
edited
Loading

solve the bug results from using shallow copy in maxMin4JSON

Could you create an issue describing the detail of the bug for this PR?

It is hard to reproduce this issue using a small data set. So, I cannot.

  1. you can create a large data set to reproduce the problem
  2. you can use a small chunk size to reproduce the problem

Another important factor is to reduce the system stack size: ulimit -s 33
#15278

@fzhedu fzhedu requested a review from SunRunAway March 11, 2020 05:29
SunRunAway
SunRunAway reviewed Mar 11, 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, please format your PR title as <package>:<discription> style.

@SunRunAway SunRunAway added sig/execution SIG execution and removed sig/execution SIG execution labels Mar 11, 2020
@fzhedu fzhedu changed the title use deep copy for maxMin4JSON Executor: use deep copy for maxMin4JSON Mar 11, 2020
@fzhedu fzhedu requested a review from wshwsh12 March 11, 2020 08:20
wshwsh12
wshwsh12 approved these changes Mar 11, 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

@fzhedu fzhedu requested a review from SunRunAway March 11, 2020 08:27
@fzhedu
Copy link
Contributor Author

fzhedu commented Mar 11, 2020

/merge

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

sre-bot commented Mar 11, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

@fzhedu merge failed.

@SunRunAway
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

/run-all-tests

@fzhedu fzhedu changed the title Executor: use deep copy for maxMin4JSON executor: use deep copy for maxMin4JSON Mar 11, 2020
@sre-bot sre-bot merged commit 34ff2b9 into pingcap:master Mar 11, 2020
@fzhedu
Copy link
Contributor Author

fzhedu commented Mar 11, 2020

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 11, 2020
@fzhedu @sre-bot
executor: use deep copy for maxMin4JSON (pingcap#15242)
18881ff
@sre-bot sre-bot mentioned this pull request Mar 11, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

cherry pick to release-2.1 in PR #15288

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 11, 2020
@fzhedu @sre-bot
cherry pick pingcap#15242 to release-3.0
896ede8 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Mar 11, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

cherry pick to release-3.0 in PR #15289

sre-bot added a commit that referenced this pull request Mar 13, 2020
@sre-bot
executor: use deep copy for maxMin4JSON (#15242) (#15289)
b6c97b7
sre-bot added a commit that referenced this pull request Mar 13, 2020
@sre-bot
executor: use deep copy for maxMin4JSON (#15242) (#15288)
8fca74f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

executor: shallow copy causes bugs in aggregation in terms of json
5 participants
@fzhedu @zz-jason @sre-bot @SunRunAway @wshwsh12