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.

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

executor: change sort to parallel #9418

Closed
wants to merge 22 commits into from
Closed

Conversation

yu34po
Copy link
Contributor

@yu34po yu34po commented Feb 22, 2019

What problem does this PR solve?

What is changed and how it works?

make sort executor to parallel process
how it works:
1.get all rows from children executor
2.start several sort worker goroutine, average split rows to every sort worker, for every sort worker sort rows of itself
3.merge the sorted rows from every sort worker

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-ansible repository
  • Need to be included in the release note

@yu34po yu34po changed the title Sortmerge executor: change sort to parallel Feb 22, 2019
@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #9418 into master will increase coverage by 0.03%.
The diff coverage is 88.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9418      +/-   ##
==========================================
+ Coverage   67.42%   67.45%   +0.03%     
==========================================
  Files         373      374       +1     
  Lines       78564    78719     +155     
==========================================
+ Hits        52968    53097     +129     
- Misses      20839    20858      +19     
- Partials     4757     4764       +7
Impacted Files Coverage Δ
executor/executor.go 68.16% <ø> (ø) ⬆️
sessionctx/variable/session.go 30.47% <100%> (+0.22%) ⬆️
executor/builder.go 83.79% <100%> (ø) ⬆️
executor/merge_sort.go 88.51% <88.51%> (ø)
ddl/delete_range.go 75.13% <0%> (-4.24%) ⬇️
expression/schema.go 93.75% <0%> (-0.79%) ⬇️
ddl/ddl_api.go 77.21% <0%> (+0.05%) ⬆️
executor/show_stats.go 89.92% <0%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4449eb0...2a1b2bc. Read the comment docs.

@morgo morgo added contribution This PR is from a community contributor. sig/execution SIG execution labels Feb 23, 2019
executor/merge_sort.go Outdated Show resolved Hide resolved
@@ -759,6 +760,9 @@ type Concurrency struct {

// IndexSerialScanConcurrency is the number of concurrent index serial scan worker.
IndexSerialScanConcurrency int

//MergeSort is the number of concurrent sort worker
MergeSortConcurrency int
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a definition for this in sessionctx/variable/sysvar.go

@XuHuaiyu XuHuaiyu requested a review from qw4990 February 25, 2019 05:52
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
sessionctx/variable/session.go Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
executor/merge_sort.go Outdated Show resolved Hide resolved
@yu34po yu34po force-pushed the sortmerge branch 3 times, most recently from 0f169a5 to 8144e5d Compare February 27, 2019 08:25
@zz-jason
Copy link
Member

zz-jason commented Aug 3, 2019

@yu34po Sorry for the late review, please update to the master branch and resolve conflicts.

@sre-bot
Copy link
Contributor

sre-bot commented Aug 3, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@zz-jason
Copy link
Member

closed due to outdated, fell free to reopen it if it's needed.

@zz-jason zz-jason closed this Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants