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

planner: improve index merge row count estimation #19040

Merged
merged 13 commits into from
Sep 27, 2020
Merged

planner: improve index merge row count estimation #19040

merged 13 commits into from
Sep 27, 2020

Conversation

time-and-fate
Copy link
Member

What problem does this PR solve?

Issue Number: close #19039

Problem Summary: see the issue for details

What is changed and how it works?

What's Changed: Now we no longer add up row counts of all partial paths of index merge as the total row count when we're in find_best_task(). We calculate the total row count by using Selectivity() to calculate the selectivity of the DNF condition of index merge when we're in DeriveStats(). Then put it in the unused CountAfterAccess field and use it directly in find_best_task().

Check List

  • Unit test
  • Integration test

Release note

  • no release note

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Aug 6, 2020
@winoros winoros added the sig/planner SIG: Planner label Aug 6, 2020
winoros
winoros reviewed Aug 13, 2020
View reviewed changes
Copy link
Member

@winoros winoros 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.

Should this one be merged after #18741?
Also, can we add some test cases to show the improvements?

@time-and-fate
Copy link
Member Author

Should this one be merged after #18741?

Yes, because this one uses Selectivity to handle DNF conditions, which is #18741 doing.

Also, can we add some test cases to show the improvements?

Sure, I'll do that after #18741 done.

@time-and-fate time-and-fate marked this pull request as ready for review September 15, 2020 11:02
@time-and-fate time-and-fate requested a review from a team as a code owner September 15, 2020 11:02
@time-and-fate time-and-fate requested review from eurekaka and removed request for a team September 15, 2020 11:02
eurekaka
eurekaka reviewed Sep 16, 2020
View reviewed changes
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/stats.go Show resolved Hide resolved
eurekaka
eurekaka reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka 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 added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 17, 2020
@time-and-fate
Copy link
Member Author

@winoros PTAL~

winoros
winoros reviewed Sep 23, 2020
View reviewed changes
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

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 23, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 23, 2020
@winoros
Copy link
Member

winoros commented Sep 23, 2020

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@time-and-fate merge failed.

@time-and-fate
Copy link
Member Author

/run-check_dev

1 similar comment
@time-and-fate
Copy link
Member Author

/run-check_dev

@time-and-fate
Copy link
Member Author

/run-all-tests

@time-and-fate
Copy link
Member Author

Please help to merge this PR. @winoros

@time-and-fate
Copy link
Member Author

/run-all-tests

@time-and-fate
Copy link
Member Author

/run-check_dev

1 similar comment
@time-and-fate
Copy link
Member Author

/run-check_dev

@eurekaka
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 20073

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@time-and-fate merge failed.

@eurekaka eurekaka merged commit 421e87c into pingcap:master Sep 27, 2020
@guo-shaoge guo-shaoge mentioned this pull request Dec 13, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@winoros winoros winoros left review comments

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

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

improve index merge row count estimation
4 participants
@time-and-fate @winoros @ti-srebot @eurekaka