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

types: convert string to MySQL BIT correctly #21310

Merged
merged 14 commits into from
Jan 18, 2021
Merged

types: convert string to MySQL BIT correctly #21310

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ce10ab7
types: convert string to MySQL BIT correctly
rebelice Nov 26, 2020
b31bb02
fix test
rebelice Nov 26, 2020
77375fd
Merge branch 'master' into dev_load_value
rebelice Nov 26, 2020
05bd9ea
add intergration test
rebelice Dec 21, 2020
1683cef
Merge branch 'master' into dev_load_value
rebelice Dec 21, 2020
b61e5c8
address comments
rebelice Dec 29, 2020
675933c
Merge branch 'master' into dev_load_value
rebelice Dec 29, 2020
48fabcf
address comment
rebelice Jan 4, 2021
6d79fb0
Merge branch 'master' into dev_load_value
ti-srebot Jan 5, 2021
caa552f
Merge branch 'master' into dev_load_value
ti-srebot Jan 5, 2021
7e4fa93
Merge branch 'master' into dev_load_value
rebelice Jan 5, 2021
f9f7a59
Merge branch 'master' into dev_load_value
rebelice Jan 6, 2021
80c38c3
Merge branch 'master' into dev_load_value
rebelice Jan 6, 2021
2c9ced7
Merge branch 'master' into dev_load_value
ti-srebot Jan 18, 2021
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
27 changes: 26 additions & 1 deletion types/datum.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,13 +1423,38 @@ func (d *Datum) convertToMysqlFloatYear(sc *stmtctx.StatementContext, target *Fi
return ret, err
}

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

please be more precise with this comment, because once the PR get merged, no one will know what the "previous processing method" is.

return BinaryLiteral(d.b).ToInt(sc)
}
return bitStr.ToInt(sc)
}

func (d *Datum) convertToMysqlBit(sc *stmtctx.StatementContext, target *FieldType) (Datum, error) {
var ret Datum
var uintValue uint64
var err error
switch d.k {
case KindString, KindBytes:
case KindBytes:
uintValue, err = BinaryLiteral(d.b).ToInt(sc)
case KindString:
// To solve issue #18681, we need consider true, false, 0, 1.
// More situations, we also consider b'0', b'1' and so on.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no mention of related behavior in the MySQL documentation, and I think there is nothing wrong with this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

s := BinaryLiteral(d.b).ToString()
if target.Flen == 1 {
switch strings.ToLower(s) {
case "true", "1":
uintValue = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

case "false", "0":
uintValue = 0
default:
uintValue, err = d.convertStringToMysqlBit(sc)
}
} else {
uintValue, err = d.convertStringToMysqlBit(sc)
}
case KindInt64:
// if input kind is int64 (signed), when trans to bit, we need to treat it as unsigned
d.k = KindUint64
Expand Down
23 changes: 23 additions & 0 deletions types/datum_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,29 @@ func prepareCompareDatums() ([]Datum, []Datum) {
return vals, vals1
}

func (ts *testDatumSuite) TestStringToMysqlBit(c *C) {
tests := []struct {
a Datum
out []byte
}{
{NewStringDatum("true"), []byte{1}},
{NewStringDatum("false"), []byte{0}},
{NewStringDatum("1"), []byte{1}},
{NewStringDatum("0"), []byte{0}},
{NewStringDatum("b'1'"), []byte{1}},
{NewStringDatum("b'0'"), []byte{0}},
}
sc := new(stmtctx.StatementContext)
sc.IgnoreTruncate = true
tp := NewFieldType(mysql.TypeBit)
tp.Flen = 1
for _, tt := range tests {
bin, err := tt.a.convertToMysqlBit(nil, tp)
c.Assert(err, IsNil)
c.Assert(bin.b, BytesEquals, tt.out)
}
}

func BenchmarkCompareDatum(b *testing.B) {
vals, vals1 := prepareCompareDatums()
sc := new(stmtctx.StatementContext)
Expand Down