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

statistics: add default value of CMSketch for Analyze (#19455) #19927

Merged
merged 5 commits into from
Sep 21, 2020
Merged

statistics: add default value of CMSketch for Analyze (#19455) #19927

merged 5 commits into from
Sep 21, 2020

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Sep 10, 2020 •
edited
Loading

cherry-pick #19455 to release-4.0

What problem does this PR solve?

Issue Number: close #19343

Problem Summary:
Add default value of CMSketch for Analyze

What is changed and how it works?

We use count / NDV in CMSketch as default value. This means count and NDV are not include topN.

Proposal: xxx

What's Changed:

  • Use count / NDV as default value.
  • In queryHashValue, if res is 0 before the noise is eliminated, the default value is not used.

Check List

Tests

  • Unit test

Side effects

Release note

  • Add default value of CMSketch for Analyze

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot mentioned this pull request Sep 10, 2020
Merged
@ti-srebot ti-srebot added component/statistics sig/execution SIG execution sig/planner SIG: Planner type/4.0-cherry-pick type/bug The issue is confirmed as a bug. labels Sep 10, 2020
@ti-srebot ti-srebot added this to the 4.0.7 milestone Sep 10, 2020
zz-jason
zz-jason reviewed Sep 11, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason 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 11, 2020
qw4990
qw4990 reviewed Sep 11, 2020
View reviewed changes
executor/analyze_test.go Outdated Show resolved Hide resolved
qw4990
qw4990 approved these changes Sep 11, 2020
View reviewed changes
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

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 11, 2020
@qw4990
Copy link
Contributor

qw4990 commented Sep 11, 2020

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @qw4990, you don't have permission to trigger auto merge event on this branch.
The version releasement is in progress.

@qw4990
Copy link
Contributor

qw4990 commented Sep 11, 2020

/run-all-tests

1 similar comment
@rebelice
Copy link
Contributor

/run-all-tests

@lilinghai
Copy link
Contributor

/build

@lilinghai
Copy link
Contributor

/run-all-tests

@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @lzmhhh123, you don't have permission to trigger auto merge event on this branch.

@SunRunAway
Copy link
Contributor

/merge

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

Your auto merge job has been accepted, waiting for:

  • 20092
  • 19972
  • 19967
  • 19967

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit 0fbe796 into pingcap:release-4.0 Sep 21, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Sep 21, 2020

Integrity check:
severity RCA symptom trigger_condition wa affect_version fix_version fields are empty

Please comment /info to get template

@ti-srebot
Copy link
Contributor Author

Please edit this comment to complete the following information

Not a bug

  1. Remove the 'type/bug' label
  2. Add notes to indicate why it is not a bug

Duplicate bug

  1. Add the 'type/duplicate' label
  2. Add the link to the original bug

Bug

Note: Make Sure that 'component', and 'severity' labels are added

1. Root Cause Analysis (RCA)

2. Symptom

3. All Trigger Conditions

4. Workaround (optional)

5. Affected versions

6. Fixed versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebelice rebelice rebelice left review comments

@zz-jason zz-jason zz-jason left review comments

@qw4990 qw4990 qw4990 approved these changes

@eurekaka eurekaka Awaiting requested review from eurekaka

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

@winoros winoros Awaiting requested review from winoros

Assignees
No one assigned
Labels
challenge-program component/statistics sig/execution SIG execution 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. type/bug The issue is confirmed as a bug. type/4.0-cherry-pick
Projects
None yet
Milestone
v4.0.7
Development

Successfully merging this pull request may close these issues.
9 participants
@ti-srebot @qw4990 @rebelice @lilinghai @lzmhhh123 @SunRunAway @sre-bot @zz-jason @seiya-annie