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

rowcodec: fix lose decimal precision during decode new row format to datum #14631

Merged
merged 3 commits into from
Feb 5, 2020
Merged

rowcodec: fix lose decimal precision during decode new row format to datum #14631

merged 3 commits into from
Feb 5, 2020

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Feb 4, 2020

What problem does this PR solve?

tk.MustExec(`drop table if exists t1`)
tk.MustExec(`create table t1(c1 decimal(6,4), primary key(c1))`)
tk.MustExec(`insert into t1 set c1 = 0.1`)
tk.MustExec(`insert into t1 set c1 = 0.1 on duplicate key update c1 = 1`)
tk.MustQuery(`select * from t1 use index(primary)`).Check(testkit.Rows(`1.0000`))

will fail due to lose precision/frac after decode from row data into datum, it will affact for kv get request that need compare datum, like batchGetChecker.

thanks @crazycs520 find this critical bug.

What is changed and how it works?

need set precision/frac for datum just like old decodeOne

and also backport #13837 modification in decode to chunk logic to new row format decode.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • impl change

Side effects

  • n/a

Related changes

  • master only

Release note

  • n/a.

This change is Reviewable

@lysu lysu added type/bugfix This PR fixes a bug. component/util labels Feb 4, 2020
@lysu lysu added this to the v4.0.0-beta.1 milestone Feb 4, 2020
@lysu
Copy link
Contributor Author

lysu commented Feb 4, 2020

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 4, 2020
Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 5, 2020
@coocood
Copy link
Member

coocood commented Feb 5, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 5, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 5, 2020

/run-all-tests

@sre-bot sre-bot merged commit 0997828 into pingcap:master Feb 5, 2020
@lysu lysu deleted the fix-new-rf-decimal-decode-miss-prec branch February 5, 2020 02:14
hsqlu pushed a commit to hsqlu/tidb that referenced this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/util 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