ddl, util/types: make converter correctly convert string to hex or bit. #3115
Conversation
This PR fix pingcap#2858 and pingcap#2717. Furthermore fix the error that unable to set default value as a hex value just like what happend in pingcap#2858. I also noticed that TiDB have some differences when compared to MySQL. create table t1 (a bit(64)); insert t1 value ('10'); select a+0 from t1; In MySQL we can get: +-------+ | a+0 | +-------+ | 12592 | +-------+ But in TiDB we will get: +-------+ | a+0 | +-------+ | 10 | +-------+ As a result of fix pingcap#2717, TiDB now behave same as MySQL.
bobotu
changed the title
|
Well done. |
|
@bobotu Please rebase your PR and do run |
ddl/ddl_api.go
Outdated
| if v.IsNull() { | ||
| return nil, nil | ||
| } else if v.Kind() == types.KindMysqlHex { |
| @@ -1044,7 +1044,17 @@ func (d *Datum) convertToMysqlYear(sc *variable.StatementContext, target *FieldT | |||
| } | |||
|
|
|||
| func (d *Datum) convertToMysqlBit(sc *variable.StatementContext, target *FieldType) (Datum, error) { | |||
| x, err := d.convertToUint(sc, target) | |||
| var ( | |||
There was a problem hiding this comment.
Newly added tests are fundamentally related to changes within this function. Parser will pass a string type Datum to this function when user input '\0' or '100', and getDefaultValue will pass a string presentation of bit to it in the end. As mentioned in commit message, MySQL treat string as binary string when convert it to bit, but TiDB treat it as number literal. This difference cause #2858 and #2717. So new tests cover the string type case and existing tests cover others cases.
|
LGTM |
|
|
||
| s.tk.MustExec("create table t_issue_2858_bit (a bit(64) default b'0')") | ||
| s.tk.MustExec("insert into t_issue_2858_bit value ()") | ||
| s.tk.MustExec(`insert into t_issue_2858_bit values (100), ('10'), ('\0')`) |
There was a problem hiding this comment.
add test for
insert into t_issue_2858_bit value('\x01')
select a + 0 from t;
In MySQL, we get: 7876657
It seems we will get 2 in TiDB with this commmit?
There was a problem hiding this comment.
Never mind it, I've test this case which is ok.
util/types/bit.go
Outdated
|
|
||
| b := hack.Slice(s) | ||
| if width == UnspecifiedBitWidth { | ||
| width = (len(b) + 7) & ^7 |
There was a problem hiding this comment.
The implementation of this func seems to be parse hex string to uint64?
If so, it seems width = len(b)*8?
There was a problem hiding this comment.
You are right. I'll fix it later.
There was a problem hiding this comment.
ok, would it be better to rename the function to ParseHex?
There was a problem hiding this comment.
ParseStringToBitValue would be better. I'll refactor the code.
util/types/bit.go
Outdated
| @@ -106,15 +106,15 @@ func ParseBit(s string, width int) (Bit, error) { | |||
| return Bit{Value: n, Width: width}, nil | |||
| } | |||
|
|
|||
| // ParseBitValue parses the binary string for bit type into uint64. | |||
| func ParseBitValue(s string, width int) (uint64, error) { | |||
| // ParseStringToBitValue parses the binary string for bit type into uint64. | |||
There was a problem hiding this comment.
Sorry I don‘t understand what you mean.
There was a problem hiding this comment.
substitute "binary string" with "string" usually written as "s/binary string/string" for short.
|
PTAL @shenli |
|
LGTM |
This PR fix #2858 and #2717. Furthermore fix the error that
unable to set default value as a hex value just like what happend in #2858.
I also noticed that TiDB have some differences when compared to MySQL.
create table t1 (a bit(64));
insert t1 value ('10');
select a+0 from t1;
In MySQL we can get:
+-------+
| a+0 |
+-------+
| 12592 |
+-------+
But in TiDB we will get:
+-------+
| a+0 |
+-------+
| 10 |
+-------+
As a result of fix #2717, TiDB now behave same as MySQL.