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.

Sign up for GitHub

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

Jump to bottom

server: fix type year has too many 0s in prepare/execute #7525

Merged
merged 3 commits into from
Aug 30, 2018
Merged

server: fix type year has too many 0s in prepare/execute #7525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92d3afb
fix too many 0s
Aug 29, 2018
0ca8947
remove extra condition
Aug 29, 2018
9a9f335
Merge branch 'master' into toomany0
jackysp Aug 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/driver_tidb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ func convertColumnInfo(fld *ast.ResultField) (ci *ColumnInfo) {
// Consider the decimal point.
ci.ColumnLength++
}
} else if fld.Column.Tp != mysql.TypeBit && fld.Column.Tp != mysql.TypeTiny {
} else if types.IsString(fld.Column.Tp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My original question still remains open. Why does types.IsString(fld.Column.Tp) work for year? I understood this is used for solving the compatibility issue we found in Navicat.
At least, it is worth a comment here.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is already explained in the PR description, we don't need to explain why old code causes some bug, because old code is removed.

Copy link
Author

Choose a reason for hiding this comment

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

Firstly, year is a number typer, so it will not multiply mysql.MaxBytesOfCharacter.
Secondly, using types.IsString(fld.Column.Tp) rather than just fld.Column.Tp != mysql.TypeYear can fix some other problem cause by this if branch.

Copy link
Author

Choose a reason for hiding this comment

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

@zhexuany , For example,

create table t (k smallint(10) unsigned zerofill);
insert into t value (10);

using jdbc test code above, TiDB and MySQL has different result.
MySQL: 0000000010
TiDB: 0000000000000000000000000000000000000010

This mean number type meet zerofill will has this problem. So we should use types.IsString(fld.Column.Tp).

Copy link
Contributor

@zhexuany zhexuany Aug 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

we don't need to explain why old code causes some bug, because old code is removed.

But we need why the fixes work?. Information still resides at PR not code itself.

When people read this code, they probably have no idea about this change. They have to go through PR which is a form of cost. I believe information has to be togehter with the code. That is why we need comment.

Copy link
Author

Choose a reason for hiding this comment

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

I will add some comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We only need check string type simply because string type has truncation problem. This comment can provide context which can help other people better understand.

// Fix issue #4540.
// The flen is a hint, not a precise value, so most client will not use the value.
// But we found in rare MySQL client, like Navicat for MySQL(version before 12) will truncate
Expand Down
22 changes: 22 additions & 0 deletions server/driver_tidb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,26 @@ func (ts tidbResultSetTestSuite) TestConvertColumnInfo(c *C) {
}
colInfo = convertColumnInfo(&resultField)
c.Assert(colInfo, DeepEquals, createColumnByTypeAndLen(mysql.TypeTiny, 1))

jackysp marked this conversation as resolved.
Show resolved Hide resolved
resultField = ast.ResultField{
Column: &model.ColumnInfo{
Name: model.NewCIStr("a"),
ID: 0,
Offset: 0,
FieldType: types.FieldType{
Tp: mysql.TypeYear,
Flag: mysql.ZerofillFlag,
Flen: 4,
Decimal: 0,
Charset: charset.CharsetBin,
Collate: charset.CollationBin,
},
Comment: "column a is the first column in table dual",
},
ColumnAsName: model.NewCIStr("a"),
TableAsName: model.NewCIStr("dual"),
DBName: model.NewCIStr("test"),
}
colInfo = convertColumnInfo(&resultField)
c.Assert(colInfo.ColumnLength, Equals, uint32(4))
}