-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
statistics: support tracking column cmsketch #35135
Conversation
Signed-off-by: yisaer <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Please follow PR Title Format:
Or if the count of mainly changed packages are more than 3, use
After you have format title, you can leave a comment |
/run-check_title |
Signed-off-by: yisaer <[email protected]>
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/ed536b2842f706994d9158858ed186d8cb407892 |
Signed-off-by: yisaer <[email protected]> make loaded atomic Signed-off-by: yisaer <[email protected]>
f29e571
to
be6d2ab
Compare
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]> add size metrics Signed-off-by: yisaer <[email protected]> Revert "add size metrics" This reverts commit 05b06a3f4f3ad98ae3f6ca389db2fc8ccc193dc4.
05b06a3
to
7cefc59
Compare
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
/run-unit-test |
statistics/histogram.go
Outdated
func (c *Column) DropEvicted() { | ||
if c.StatsVer < Version2 { | ||
c.CMSketch = nil | ||
c.Loaded = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Loaded means if the histogram, the topn and the cm sketch are loaded fully". Before the pr, we assume either all three are loaded or none of the three are loaded. Now we break the assumption. It may bring some problems. For example, (*Column).IsLoaded
returns c.Loaded || c.notNullCount() > 0
. It seems that we assume c.Loaded
is always equal to c.notNullCount() > 0
and will remove c.notNullCount() > 0
in the future. Now if the cm sketch is evicted in ver1 case, c.Loaded
is false while c.notNullCount() > 0
is true. cc @time-and-fate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will revise loaded in next pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyifangreeneyes is correct.
There're mainly two usages of IsLoaded
:
- help to check if we can use the non-pseudo estimation logic for this
Column
. - help to check if stats in this
Column
needs loading.
And of course, they should be essentially the same thing before this PR.
It seems that they become different in this PR, so we may need to introduce a new field or change IsLoaded
to represent more states to handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the revised loaded updated in #35361
Signed-off-by: yisaer <[email protected]>
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 532e2df
|
/run-unit-test |
/run-build |
/run-mysql-test |
1 similar comment
/run-mysql-test |
Signed-off-by: yisaer <[email protected]>
…pport_column_cmsketch
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 1b8a804
|
/run-all-tests |
/run-unit-test |
Signed-off-by: yisaer <[email protected]>
c1b7af9
to
7bf2426
Compare
/merge |
This pull request has been accepted and is ready to merge. Commit hash: ff4158d
|
TiDB MergeCI notify✅ Well Done! New fixed [2] after this pr merged.
|
Signed-off-by: yisaer [email protected]
What problem does this PR solve?
Issue Number: ref #34052
Problem Summary:
What is changed and how it works?
Check List
Tests
statscache test report
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.