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: limit the number of top-n items #11906

Merged
merged 4 commits into from
Aug 28, 2019
Merged

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Aug 28, 2019

What problem does this PR solve?

mysql>analyze table t_wide
(1105, u’statement count 5001 exceeds the transaction limitation, autocommit = true’)

What is changed and how it works?

The root cause is that analyze will generate topn items much more than expected, because we regard one item as topn as long as it occurs more than last times and does not limit the number of topn.

This PR fixes it by limit the number of topn.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

Release note

  • limit the number of top-n items for analyze statement.

@codecov
Copy link

codecov bot commented Aug 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c599333). Click here to learn what that means.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #11906   +/-   ##
===========================================
  Coverage          ?   81.2248%           
===========================================
  Files             ?        438           
  Lines             ?      94236           
  Branches          ?          0           
===========================================
  Hits              ?      76543           
  Misses            ?      12229           
  Partials          ?       5464

statistics/cmsketch.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 Aug 28, 2019
@alivxxx alivxxx requested a review from lzmhhh123 August 28, 2019 09:14
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. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

/run-all-tests

@sre-bot sre-bot merged commit ab8346e into pingcap:master Aug 28, 2019
@alivxxx alivxxx deleted the cmsketch branch August 28, 2019 09:24
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/statistics status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants