types: convert string to MySQL BIT correctly #21310
Conversation
|
/label component/expression, sig/execution |
|
/label needs-cherry-pick-4.0 |
|
/cc @ichn-hu, @lzmhhh123 |
| if target.Flen == 1 { | ||
| switch strings.ToLower(s) { | ||
| case "true", "1": | ||
| uintValue = 1 |
There was a problem hiding this comment.
In mysql 8.0:
mysql > select true, b'1', cast(true as binary);
+------+------------+--------------------------------------------+
| true | b'1' | cast(true as binary) |
+------+------------+--------------------------------------------+
| 1 | 0x01 | 0x31 |
+------+------------+--------------------------------------------+
1 row in set (0.00 sec)Maybe we should handle it in another place.
There was a problem hiding this comment.
This sentence of SQL has nothing to do with convert. It can be seen from the following that these values can be converted correctly, but they are returned as binary, so the output to MySQL Client may not be displayed.
MySQL [test]> select true, HEX(b'1'), HEX(cast(true as binary));
+------+-----------+---------------------------+
| TRUE | HEX(b'1') | HEX(cast(true as binary)) |
+------+-----------+---------------------------+
| 1 | 01 | 31 |
+------+-----------+---------------------------+
1 row in set (0.001 sec)
MySQL [test]> select true, b'1', cast(true as binary);
+------+------+----------------------+
| TRUE | b'1' | cast(true as binary) |
+------+------+----------------------+
| 1 | | 1 |
+------+------+----------------------+
1 row in set (0.001 sec)I think these are two different problems, and it's best to solve it in another PR
executor/write_test.go
Outdated
| _, err = tk.Exec("load data infile '/tmp/nonexistence.csv' into table load_data_test") | ||
| c.Assert(err, NotNil) | ||
| _, err = tk.Exec("load data local infile '/tmp/nonexistence.csv' replace into table load_data_test") | ||
| c.Assert(err, NotNil) | ||
| tk.MustExec("load data local infile '/tmp/nonexistence.csv' ignore into table load_data_test") |
There was a problem hiding this comment.
how are these tests relate to your change?
types/datum.go
Outdated
| func (d *Datum) convertStringToMysqlBit(sc *stmtctx.StatementContext) (uint64, error) { | ||
| bitStr, err := ParseBitStr(BinaryLiteral(d.b).ToString()) | ||
| if err != nil { | ||
| // This is to be compatible with the previous processing method. |
There was a problem hiding this comment.
please be more precise with this comment, because once the PR get merged, no one will know what the "previous processing method" is.
types/datum.go
Outdated
| // To solve issue #18681, we need consider true, false, 0, 1. | ||
| // More situations, we also consider b'0', b'1' and so on. |
There was a problem hiding this comment.
I guess this fix is not specific to 18681, and the code itself is self-explaining, if you want to add comment, it is better to find if there is a reference to MySQL's doc.
There was a problem hiding this comment.
There is no mention of related behavior in the MySQL documentation, and I think there is nothing wrong with this comment
There was a problem hiding this comment.
What I am saying is that this comment is not comprehensive enough if not redundant, you don't fix code because of an issue report, you fix code because you want proper behavior, that's why I am asking for a doc reference.
I am not against referencing to an issue here, but I would like a comment like this:
// For single bit value, we take string like "true", "1" as 1, and "false", "0" as 0,
// this behavior is not documented in MySQL, but it behaves so, for more information, see issue #18681
executor/write_test.go
Outdated
| tests := []testCase{ | ||
| // data1 = nil, data2 != nil | ||
| {nil, []byte("true\tfalse\t0\t1\n"), []string{"1|0|0|1"}, nil, "Records: 1 Deleted: 0 Skipped: 0 Warnings: 0"}, | ||
| } |
There was a problem hiding this comment.
Probably you can consider intergrate this test into the following TestLoadData function?
There was a problem hiding this comment.
May not work because the table structure is not the same.
| createSQL := `drop table if exists load_data_test; | ||
| create table load_data_test (a bit(1),b bit(1),c bit(1),d bit(1));` | ||
| tk.MustExec(createSQL) | ||
| tk.MustExec("load data local infile '/tmp/nonexistence.csv' ignore into table load_data_test") |
There was a problem hiding this comment.
this line is still irrelevant?
There was a problem hiding this comment.
No, this is initialization.
types/datum.go
Outdated
| // To solve issue #18681, we need consider true, false, 0, 1. | ||
| // More situations, we also consider b'0', b'1' and so on. |
There was a problem hiding this comment.
What I am saying is that this comment is not comprehensive enough if not redundant, you don't fix code because of an issue report, you fix code because you want proper behavior, that's why I am asking for a doc reference.
I am not against referencing to an issue here, but I would like a comment like this:
// For single bit value, we take string like "true", "1" as 1, and "false", "0" as 0,
// this behavior is not documented in MySQL, but it behaves so, for more information, see issue #18681
|
@rebelice merge failed. |
|
/merge |
|
/run-all-tests |
|
@rebelice merge failed. |
|
/run-all-tests |
|
/run-all-tests |
|
/run-unit-test |
|
/run-all-tests |
|
/merge |
|
/run-all-tests |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-4.0 in PR #22420 |
Signed-off-by: ti-srebot <[email protected]>
What problem does this PR solve?
Issue Number: close #18681
Problem Summary:
LOAD DATAdoes not take effect on BIT. BecauseLOAD DATAregards all input as strings and TiDB cannot convert any string to bit correctlyWhat is changed and how it works?
What's Changed:
true,false,0,1Related changes
Check List
Tests
Side effects
Release note