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

planner: consider disk cost in Sort #14708

Merged
merged 7 commits into from
Feb 12, 2020
Merged

Conversation

wshwsh12
Copy link
Contributor

What problem does this PR solve?

Consider disk cost in Sort.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

Release note

@wshwsh12 wshwsh12 added the sig/planner SIG: Planner label Feb 10, 2020
@wshwsh12 wshwsh12 requested a review from a team as a code owner February 10, 2020 07:07
@ghost ghost removed their request for review February 10, 2020 07:07
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -841,19 +841,45 @@ func (p *PhysicalTopN) allColsFromSchema(schema *expression.Schema) bool {
return len(schema.ColumnsIndices(cols)) > 0
}

func (p *PhysicalSort) avgRowSize(schema *expression.Schema) (size float64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse the avgRowSize function of the PhysicalHashJoin?

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Feb 12, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 12, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 12, 2020

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/rebuild

@wshwsh12
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 12, 2020

/run-all-tests

@sre-bot sre-bot merged commit 6e867b7 into pingcap:master Feb 12, 2020
@wshwsh12 wshwsh12 deleted the sort_cost branch April 21, 2020 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants