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

stats: incremental analyze for index with feedback updates #10355

Merged
merged 8 commits into from
May 8, 2019

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented May 5, 2019

What problem does this PR solve?

Support incremental analyze for the index with feedback updates.

What is changed and how it works?

For index with feedback updates, we will analyze from the last analyze position and merges it with the previous stats.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • None

@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #10355 into master will decrease coverage by 0.0001%.
The diff coverage is 75.7009%.

@@               Coverage Diff               @@
##             master    #10355        +/-   ##
===============================================
- Coverage   77.3741%   77.374%   -0.0002%     
===============================================
  Files           412       412                
  Lines         85588     85636        +48     
===============================================
+ Hits          66223     66260        +37     
- Misses        14339     14349        +10     
- Partials       5026      5027         +1

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@alivxxx
Copy link
Contributor Author

alivxxx commented May 6, 2019

/run-all-tests

@alivxxx
Copy link
Contributor Author

alivxxx commented May 6, 2019

/run-unit-test

@qw4990 qw4990 self-requested a review May 6, 2019 08:44
statistics/cmsketch.go Outdated Show resolved Hide resolved
executor/analyze.go Show resolved Hide resolved
statistics/histogram.go Outdated Show resolved Hide resolved
executor/builder.go Show resolved Hide resolved
statistics/handle/update.go Outdated Show resolved Hide resolved
statistics/handle/handle.go Outdated Show resolved Hide resolved
statistics/histogram.go Show resolved Hide resolved
@eurekaka
Copy link
Contributor

eurekaka commented May 6, 2019

Can we remove the Flag field, and always assume that there is feedback update, so we call TruncateHistogram in all cases(even if there is no feedback update) to simplify the code?

@alivxxx
Copy link
Contributor Author

alivxxx commented May 6, 2019

@eurekaka That means we need to analyze at least 1/256 rows, we can do it better when there are no feedbacks.

@eurekaka
Copy link
Contributor

eurekaka commented May 7, 2019

That means we need to analyze at least 1/256 rows

How does this come from?

@alivxxx
Copy link
Contributor Author

alivxxx commented May 7, 2019

How does this come from?

Since we have 256 buckets in a histogram and they are equal depth, so truncate one bucket would lead to analyze at least 1/256 rows. Also, the last analyze pos may not be available for old stats.
@eurekaka

statistics/cmsketch.go Outdated Show resolved Hide resolved
@alivxxx alivxxx requested review from eurekaka, qw4990 and winoros and removed request for qw4990 May 7, 2019 11:42
executor/analyze_test.go Outdated Show resolved Hide resolved
statistics/histogram.go Outdated Show resolved Hide resolved
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

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label May 8, 2019
@alivxxx alivxxx requested review from zz-jason and qw4990 May 8, 2019 05:35
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990
Copy link
Contributor

qw4990 commented May 8, 2019

/run-all-tests

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 8, 2019
@alivxxx alivxxx merged commit b0549b7 into pingcap:master May 8, 2019
@alivxxx alivxxx deleted the incremental branch May 8, 2019 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants