-
Notifications
You must be signed in to change notification settings - Fork 489
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
types: port 'HasCharset' function from MySQL #270
Conversation
types/field_type.go
Outdated
// statements(like `SHOW CREATE TABLE`) from attaching a CHARACTER SET clause to the column. | ||
func HasCharset(ft *FieldType) bool { | ||
switch ft.Tp { | ||
case mysql.TypeVarchar, mysql.TypeString, mysql.TypeVarString, mysql.TypeBlob, mysql.TypeTinyBlob, mysql.TypeMediumBlob, mysql.TypeLongBlob: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is too long, maybe we could spilit it into two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
case
mysql.TypeVarchar,
mysql.TypeString,
mysql.TypeVarString,
mysql.TypeBlob,
mysql.TypeTinyBlob,
mysql.TypeMediumBlob,
mysql.TypeLongBlob:
return !mysql.HasBinaryFlag(ft.Flag)
case
mysql.TypeEnum,
mysql.TypeSet:
return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to keep consistent with Golang is enough.
For example:
https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/go/types/return.go#L17-L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, I think there is something need attention. @xiekeyi98
create table t (
a bit(11),
b int,
c int(3),
d float,
e double,
f decimal(10,2),
g numeric(10,2),
h date,
i datetime,
j timestamp,
k time,
m year,
c0 char(10),
c1 varchar(10),
c2 binary(10),
c3 varbinary(10),
c4 blob(50),
c5 text(50),
c6 ENUM('value1','value2'),
c7 SET('value1','value2'),
c8 JSON
) charset="utf8mb4"
In MySQL:
test> show full columns from t;
+-------+-------------------------+--------------------+------+-----+-------------------+-----------------------------+---------------------------------+---------+
| Field | Type | Collation | Null | Key | Default | Extra | Privileges | Comment |
+-------+-------------------------+--------------------+------+-----+-------------------+-----------------------------+---------------------------------+---------+
| a | bit(11) | <null> | YES | | <null> | | select,insert,update,references | |
| b | int(11) | <null> | YES | | <null> | | select,insert,update,references | |
| c | int(3) | <null> | YES | | <null> | | select,insert,update,references | |
| d | float | <null> | YES | | <null> | | select,insert,update,references | |
| e | double | <null> | YES | | <null> | | select,insert,update,references | |
| f | decimal(10,2) | <null> | YES | | <null> | | select,insert,update,references | |
| g | decimal(10,2) | <null> | YES | | <null> | | select,insert,update,references | |
| h | date | <null> | YES | | <null> | | select,insert,update,references | |
| i | datetime | <null> | YES | | <null> | | select,insert,update,references | |
| j | timestamp | <null> | NO | | CURRENT_TIMESTAMP | on update CURRENT_TIMESTAMP | select,insert,update,references | |
| k | time | <null> | YES | | <null> | | select,insert,update,references | |
| m | year(4) | <null> | YES | | <null> | | select,insert,update,references | |
| c0 | char(10) | utf8mb4_general_ci | YES | | <null> | | select,insert,update,references | |
| c1 | varchar(10) | utf8mb4_general_ci | YES | | <null> | | select,insert,update,references | |
| c2 | binary(10) | <null> | YES | | <null> | | select,insert,update,references | |
| c3 | varbinary(10) | <null> | YES | | <null> | | select,insert,update,references | |
| c4 | tinyblob | <null> | YES | | <null> | | select,insert,update,references | |
| c5 | tinytext | utf8mb4_general_ci | YES | | <null> | | select,insert,update,references | |
| c6 | enum('value1','value2') | utf8mb4_general_ci | YES | | <null> | | select,insert,update,references | |
| c7 | set('value1','value2') | utf8mb4_general_ci | YES | | <null> | | select,insert,update,references | |
| c8 | json | <null> | YES | | <null> | | select,insert,update,references | |
+-------+-------------------------+--------------------+------+-----+-------------------+-----------------------------+---------------------------------+---------+
In TiDB
+-------+-------------------------+-------------+------+-----+---------+-------+---------------------------------+---------+
| Field | Type | Collation | Null | Key | Default | Extra | Privileges | Comment |
+-------+-------------------------+-------------+------+-----+---------+-------+---------------------------------+---------+
| a | bit(11) | binary | YES | | <null> | | select,insert,update,references | |
| b | int(11) | binary | YES | | <null> | | select,insert,update,references | |
| c | int(3) | binary | YES | | <null> | | select,insert,update,references | |
| d | float | binary | YES | | <null> | | select,insert,update,references | |
| e | double | binary | YES | | <null> | | select,insert,update,references | |
| f | decimal(10,2) | binary | YES | | <null> | | select,insert,update,references | |
| g | decimal(10,2) | binary | YES | | <null> | | select,insert,update,references | |
| h | date | NULL | YES | | <null> | | select,insert,update,references | |
| i | datetime | NULL | YES | | <null> | | select,insert,update,references | |
| j | timestamp | NULL | YES | | <null> | | select,insert,update,references | |
| k | time | NULL | YES | | <null> | | select,insert,update,references | |
| m | year(4) | NULL | YES | | <null> | | select,insert,update,references | |
| c0 | char(10) | utf8mb4_bin | YES | | <null> | | select,insert,update,references | |
| c1 | varchar(10) | utf8mb4_bin | YES | | <null> | | select,insert,update,references | |
| c2 | binary(10) | binary | YES | | <null> | | select,insert,update,references | |
| c3 | varbinary(10) | binary | YES | | <null> | | select,insert,update,references | |
| c4 | blob | binary | YES | | <null> | | select,insert,update,references | |
| c5 | text | utf8mb4_bin | YES | | <null> | | select,insert,update,references | |
| c6 | enum('value1','value2') | utf8mb4_bin | YES | | <null> | | select,insert,update,references | |
| c7 | set('value1','value2') | utf8mb4_bin | YES | | <null> | | select,insert,update,references | |
| c8 | json | binary | YES | | <null> | | select,insert,update,references | |
+-------+-------------------------+-------------+------+-----+---------+-------+---------------------------------+---------+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
Maybe something should be refactored.
Thanks for your guidance.
❤️
addressed, thank you @xiekeyi98 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,well done.
What problem does this PR solve?
This PR aims to solve the some known show charset/collation issues in TiDB, for example, pingcap/tidb#9837
What is changed and how it works?
A
HasCharset
function is ported from MySQL source, for example:Field::has_charset()
Field_string::has_charset()
This function can be used in the procedure of some show-schema executions, for example, in MySQL it decides where a given column's
Collation
result isNULL
inshow full columns
statement:sql_show.cc::store_column_type()
Check List
Tests
Code changes
Side effects
Related changes