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

ddl: support modify decimal column precision in some cases. #14617

Closed
wants to merge 25 commits into from

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Feb 4, 2020

background

PR #10433 prohibit modify decimal column precision, because change decimal column precision may cause data inconsistency when there is an index covering the decimal column.

What problem does this PR solve?

After think over, we can support modify a decimal column when satisfying the following conditions:

  • No index covers the modified decimal column.
  • Only support enlarges the decimal precision.
    • modify decimal(5,2) to decimal(10,5) is ok.
    • modify decimal(5,2) to decimal(4, 2) is not support.
    • modify decimal(5,2) to decimal(5, 3) is not support, because it may cause the column out range value error.

After TiDB supports all column type changes, there will be no such restrictions.

eg:

>create table t (a decimal(5,2),index(a));
Query OK
>alter table t change a a decimal(4,2);
(8200, u"Unsupported modify column: can't change decimal column precision with index covered now")
>alter table t drop index a;
Query OK
>alter table t change a a decimal(4,2);
(8200, u'Unsupported modify column: length 4 is less than origin 5')
>alter table t change a a decimal(5,3);
(8200, u'Unsupported modify column: modify original decimal(5,2) to decimal(5,3) may cause out of range value error')
>alter table t change a a decimal(10,5);
Query OK

What is changed and how it works?

Below is an example if only change ddl pkg to support modify decimal precision :

  1. The original decimal precision is decimal(2,1), and write the data 1.1
  2. modify the decimal precision to decimal(4,2), and get the data 1.1, but the expect data is 1.10.

Below is sql example:

test> create table t (a decimal(2,1));
test> insert into t set a=1.1;
test> select * from t;
+-----+
| a   |
+-----+
| 1.1 |
+-----+
test> alter table t change a a decimal(4,2);
test> select * from t;
+-----+
| a   |
+-----+
| 1.1 |   -- the value should be 1.10
+-----+

Since the decimal already encodes the precision info, So after modifying the decimal column precision, we must convert the decimal value to the new precision when reading the old decimal value.

So Also need to change the decode logical, both in TiDB and TiKV.

Related TiKV PR tikv/tikv#6532.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch?
  • Need to update the documentation

Release note

  • Support modify decimal column precision in the case like below:
    • No index covers the modified decimal column.
    • Only support enlarges the decimal precision.

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

@zimulala @bb7133 @coocood PTAL

@bb7133
Copy link
Member

bb7133 commented Feb 4, 2020

Support modify decimal column precision in some cases.

Please update the release note by adding descriptions to the cases.

util/codec/codec.go Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-unit-test

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 10, 2020
@crazycs520 crazycs520 requested a review from a team as a code owner April 1, 2020 11:53
@ghost ghost requested review from XuHuaiyu and removed request for a team April 1, 2020 11:53
@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=pr/6532

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=pr/6532

@@ -172,10 +175,18 @@ func TruncateIndexValuesIfNeeded(tblInfo *model.TableInfo, idxInfo *model.IndexI
v.SetString(v.GetString(), tblInfo.Columns[ic.Offset].Collate)
}
}
case types.KindMysqlDecimal:
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this?

@devon-ye
Copy link

Which version started to support this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/executor sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants