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: fix histogram bound overflow error #8984

Merged
merged 3 commits into from
Jan 9, 2019
Merged

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Jan 8, 2019

What problem does this PR solve?

When we have a table with unsigned primary key and have a query like select count(*) from t, the scan range of query will be [-inf, +inf], like a signed primary key, not [0,+inf](it should be another problem though). So when updating the histogram using feedback, the -inf value will be stored into uint64, which will overflow the histogram value ranges. Then when reading stats from a table, there will be an error message like Error occurred when read table stats for table sbtest1. The error message is [types:1690]constant 18446744071562067968 overflows int.

What is changed and how it works?

  • Get the min and max allowd value using the histogram value type, not the feedback datum kind, to avoid the miss matched type.
  • When decoding the query feedback for primary key, we will store the value using the correct type(signed or unsigned).

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

@codecov-io
Copy link

codecov-io commented Jan 8, 2019

Codecov Report

Merging #8984 into master will increase coverage by 0.03%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8984      +/-   ##
==========================================
+ Coverage   67.51%   67.55%   +0.03%     
==========================================
  Files         363      363              
  Lines       75264    75270       +6     
==========================================
+ Hits        50818    50851      +33     
+ Misses      19949    19929      -20     
+ Partials     4497     4490       -7
Impacted Files Coverage Δ
statistics/update.go 84.16% <100%> (ø) ⬆️
statistics/feedback.go 73.32% <72.09%> (+1.02%) ⬆️
statistics/histogram.go 82.34% <0%> (+0.39%) ⬆️
executor/join.go 78.29% <0%> (+0.4%) ⬆️
expression/schema.go 94.95% <0%> (+0.84%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️
ddl/delete_range.go 74.28% <0%> (+1.71%) ⬆️
statistics/scalar.go 82.88% <0%> (+1.8%) ⬆️
store/mockstore/mocktikv/pd.go 58.97% <0%> (+7.69%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c68ee73...f512fea. Read the comment docs.

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

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 8, 2019
@breezewish
Copy link
Member

/run-integration-common-test

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

@zz-jason
Copy link
Member

zz-jason commented Jan 9, 2019

/run-all-tests

@winoros winoros merged commit f209843 into pingcap:master Jan 9, 2019
@alivxxx alivxxx deleted the overflow branch January 9, 2019 02:59
@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 9, 2019
alivxxx added a commit to alivxxx/tidb that referenced this pull request Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/statistics 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.

5 participants