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

admin: fix admin check table compare bug #7818

Merged
merged 2 commits into from
Sep 30, 2018

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Sep 29, 2018

What problem does this PR solve?

SQL:

drop table if exists td1;
CREATE TABLE td1 (c2 INT NULL DEFAULT '70');
INSERT INTO td1 SET c2 = '5';
ALTER TABLE td1 ADD COLUMN c4 DECIMAL(12,8) NULL DEFAULT '213.41598062';
ALTER TABLE td1 ADD INDEX id2 (c4) ;
ADMIN CHECK TABLE td1;

root cause
decimal type is not reflect.DeepEqual between orginal datum and the datum after codec.encode and decode.
eg: 0.1234

origin:
&types.MyDecimal{digitsInt:1, digitsFrac:4, resultFrac:4, negative:false, wordBuf:[9]int32{0, 123400000, 0, 0, 0, 0, 0, 0, 0}} !=
after codec encode and decode:
&types.MyDecimal{digitsInt:0, digitsFrac:4, resultFrac:4, negative:false, wordBuf:[9]int32{123400000, 0, 0, 0, 0, 0, 0, 0, 0}}

What is changed and how it works?

Use Datum. CompareDatum() to replace reflect.DeepEqual in admin check table when datum type is decimal.

Check List

Tests

  • Unit test

Code changes

Side effects

  • Possible performance regression

Related changes

@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label Sep 29, 2018
@crazycs520
Copy link
Contributor Author

/run-all-tests

@shenli
Copy link
Member

shenli commented Sep 29, 2018

LGTM

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu merged commit bb8ba44 into pingcap:master Sep 30, 2018
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants