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

type: make decimal default precision visible in show create table #7667

Merged
merged 8 commits into from Sep 12, 2018
Merged

type: make decimal default precision visible in show create table #7667

merged 8 commits into from Sep 12, 2018

Conversation

imtbkcat
Copy link

What problem does this PR solve?

Fix #7665 to make jdbc test happy.

What is changed and how it works?

Add code to make default decimal precision visible in show create table.
See #7665 for detail.

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat
Copy link
Author

/run-all-tests

}
suffix += ")"
} else {
suffix = "(10,0)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

ok

@imtbkcat
Copy link
Author

PTAL @lysu

@imtbkcat
Copy link
Author

/run-all-tests

lysu
lysu previously approved these changes Sep 11, 2018
Copy link
Collaborator

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 11, 2018
}
suffix += ")"
} else {
suffix = fmt.Sprintf("(%d,0)", defaultFlen)
}
Copy link
Member

Choose a reason for hiding this comment

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

how about

if isFlenNotDefault {
} else {
}
if isDecimalNotDefault {
} else {
}

}
suffix += ")"
} else {
suffix = fmt.Sprintf("(%d,0)", defaultFlen)
Copy link
Member

Choose a reason for hiding this comment

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

It would be simpler if calculate the displayFlen and displayDecimal first, then just Sprintf once.

suffix = fmt.Sprintf("(%d,%d)", displayFlen, displayDecimal)

Copy link
Contributor

Choose a reason for hiding this comment

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

When the type is mysql.TypeNewDecimal, why still need to check isFlenNotDefault or isDecimalNotDefault? What about just using suffix = fmt.Sprintf("(%d,%d)", displayFlen, displayDecimal) ?

Copy link
Member

Choose a reason for hiding this comment

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

@winkyao , have you checked it with MySQL?

Copy link
Author

Choose a reason for hiding this comment

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

I think just use suffix = fmt.Sprintf("(%d,%d)", displayFlen, displayDecimal) is okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

When it is a decimal type, mysql show decimal length in show create table anyway. @jackysp

@@ -231,8 +231,12 @@ func (ft *FieldType) CompactStr() string {
suffix = fmt.Sprintf("(%d", displayFlen)
if isDecimalNotDefault {
Copy link
Member

Choose a reason for hiding this comment

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

how about:

if isFlenNotDefault {
	prefix = xxx
	break
}
if isDecimalNotDefault {
	prefix = xxx
	break
}
prefix = xx

@imtbkcat
Copy link
Author

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

1 similar comment
@imtbkcat
Copy link
Author

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

@imtbkcat
Copy link
Author

PTAL @zz-jason @jackysp @coocood

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

@jackysp jackysp merged commit b58a977 into pingcap:master Sep 12, 2018
@imtbkcat imtbkcat deleted the fixcreatetable branch September 12, 2018 05:00
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

7 participants