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

infoschema: fix default NUMBER_SCALE value of float type #7602

Merged
merged 6 commits into from
Sep 11, 2018
Merged

infoschema: fix default NUMBER_SCALE value of float type #7602

merged 6 commits into from
Sep 11, 2018

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Sep 4, 2018

What problem does this PR solve?

Fix wrong value for default numberScale value for float and double type.

Test Case:

create table floatschema(a float, b double(7, 3));
select NUMERIC_SCALE from information_schema.COLUMNS where table_name='floatschema';

MySQL:

mysql> select NUMERIC_SCALE from information_schema.COLUMNS where 
table_name='floatschema';
+---------------+
| NUMERIC_SCALE |
+---------------+
|          NULL |
|             3 |
+---------------+
2 rows in set (0.01 sec)

TiDB:

+----------------------+
| NUMERIC_SCALE        |
+----------------------+
| 18446744073709551615 |
|                    3 |
+----------------------+
2 rows in set (0.01 sec)

This is because the default value of NUMERIC_SCALE for float and double type is -1, but NUMERIC_SCALE is a UNSIGNED LONG.

What is changed and how it works?

add code to make NUMERIC_SCALE same as MySQL.

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

-No

@@ -1006,24 +1010,24 @@ func dataForTableConstraints(schemas []*model.DBInfo) [][]types.Datum {
func dataForPseudoProfiling() [][]types.Datum {
var rows [][]types.Datum
row := types.MakeDatums(
0, // QUERY_ID
Copy link
Member

Choose a reason for hiding this comment

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

Please update your golang version to 1.11.

@@ -1006,24 +1010,24 @@ func dataForTableConstraints(schemas []*model.DBInfo) [][]types.Datum {
func dataForPseudoProfiling() [][]types.Datum {
var rows [][]types.Datum
row := types.MakeDatums(
0, // QUERY_ID
Copy link
Member

@jackysp jackysp Sep 4, 2018

Choose a reason for hiding this comment

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

Please update your golang version to 1.11, then format again.

@imtbkcat
Copy link
Author

imtbkcat commented Sep 4, 2018

PTAL @jackysp

@imtbkcat
Copy link
Author

imtbkcat commented Sep 4, 2018

/run-all-tests

@shenli
Copy link
Member

shenli commented Sep 5, 2018

@imtbkcat Please fix the CI.

@imtbkcat
Copy link
Author

/run-all-tests

@imtbkcat
Copy link
Author

@shenli ok.

@lysu
Copy link
Collaborator

lysu commented Sep 10, 2018

/run-all-tests tidb-test=pr/619

1 similar comment
@imtbkcat
Copy link
Author

/run-all-tests tidb-test=pr/619

@coocood
Copy link
Member

coocood commented Sep 10, 2018

LGTM

@imtbkcat imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 10, 2018
@imtbkcat
Copy link
Author

/run-all-tests tidb-test=pr/619

@coocood
Copy link
Member

coocood commented Sep 10, 2018

/run-all-tests tidb-test=pr/619

@coocood
Copy link
Member

coocood commented Sep 10, 2018

/run-all-tests tidb-test=pr/619

@imtbkcat
Copy link
Author

/run-all-tests

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 zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 11, 2018
@zz-jason zz-jason merged commit 4423937 into pingcap:master Sep 11, 2018
@imtbkcat imtbkcat deleted the jdbc branch September 11, 2018 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants