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.

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

ddl, util/types: make converter correctly convert string to hex or bit. #3115

Merged
merged 6 commits into from
Apr 25, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
7 changes: 7 additions & 0 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package ddl

import (
"fmt"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -353,9 +354,15 @@ func getDefaultValue(ctx context.Context, c *ast.ColumnOption, tp byte, fsp int)
if err != nil {
return nil, errors.Trace(err)
}

if v.IsNull() {
return nil, nil
}

if v.Kind() == types.KindMysqlHex {
return strconv.FormatInt(v.GetMysqlHex().Value, 10), nil
}

return v.ToString()
}

Expand Down
18 changes: 18 additions & 0 deletions ddl/ddl_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1093,3 +1093,21 @@ out:
expected := fmt.Sprintf("%d %d", updateCnt, 3)
s.tk.MustQuery("select c2, c3 from tnn where c1 = 99").Check(testkit.Rows(expected))
}

func (s *testDBSuite) TestIssue2858And2717(c *C) {
defer testleak.AfterTest(c)()
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("use " + s.schemaName)

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')`)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind it, I've test this case which is ok.

s.tk.MustQuery("select a+0 from t_issue_2858_bit").Check([][]interface{}{{0}, {100}, {0x3130}, {0}})
s.tk.MustExec(`alter table t_issue_2858_bit alter column a set default '\0'`)

s.tk.MustExec("create table t_issue_2858_hex (a int default 0x123)")
s.tk.MustExec("insert into t_issue_2858_hex value ()")
s.tk.MustExec("insert into t_issue_2858_hex values (123), (0x321)")
s.tk.MustQuery("select a from t_issue_2858_hex").Check([][]interface{}{{0x123}, {123}, {0x321}})
s.tk.MustExec(`alter table t_issue_2858_hex alter column a set default 0x321`)
}
31 changes: 31 additions & 0 deletions util/types/bit.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"strings"

"github.com/juju/errors"
"github.com/pingcap/tidb/util/hack"
)

// Bit is for mysql bit type.
Expand Down Expand Up @@ -104,3 +105,33 @@ func ParseBit(s string, width int) (Bit, error) {

return Bit{Value: n, Width: width}, nil
}

// ParseStringToBitValue parses the binary string for bit type into uint64.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/binary string/string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don‘t understand what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

substitute "binary string" with "string" usually written as "s/binary string/string" for short.

func ParseStringToBitValue(s string, width int) (uint64, error) {
if len(s) == 0 {
return 0, errors.Errorf("invalid empty string for parsing bit value")
}

b := hack.Slice(s)
if width == UnspecifiedBitWidth {
width = len(b) * 8
}
if width == 0 {
width = MinBitWidth
}
if width < MinBitWidth || width > MaxBitWidth {
return 0, errors.Errorf("invalid display width for bit type, must in [1, 64], but %d", width)
}

var n uint64
l := len(b)
for i := range b {
n += uint64(b[l-i-1]) << uint(i*8)
}

if n > (uint64(1)<<uint64(width))-1 {
return 0, errors.Errorf("bit %s is too long for width %d", s, width)
}

return n, nil
}
4 changes: 4 additions & 0 deletions util/types/bit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ func (s *testBitSuite) TestBit(c *C) {
c.Assert(b.ToNumber(), Equals, float64(t.Number))
c.Assert(b.String(), Equals, t.String)
c.Assert(b.ToString(), Equals, t.BitString)

n, err := ParseStringToBitValue(t.BitString, t.Width)
c.Assert(err, IsNil)
c.Assert(n, Equals, uint64(t.Number))
}

tblErr := []struct {
Expand Down
5 changes: 3 additions & 2 deletions util/types/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,12 @@ func (s *testTypeConvertSuite) TestConvertType(c *C) {

// For TypeBit
ft = NewFieldType(mysql.TypeBit)
ft.Flen = 8
ft.Flen = 24
v, err = Convert("100", ft)
c.Assert(err, IsNil)
c.Assert(v, Equals, Bit{Value: 100, Width: 8})
c.Assert(v, Equals, Bit{Value: 3223600, Width: 24})

ft.Flen = 8
v, err = Convert(Hex{Value: 100}, ft)
c.Assert(err, IsNil)
c.Assert(v, Equals, Bit{Value: 100, Width: 8})
Expand Down
12 changes: 11 additions & 1 deletion util/types/datum.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Copy link
Member

Choose a reason for hiding this comment

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

Any unit test covers this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

x Datum
err error
)
if d.Kind() == KindString {
var n uint64
n, err = ParseStringToBitValue(d.GetString(), target.Flen)
x = NewUintDatum(n)
} else {
x, err = d.convertToUint(sc, target)
}
if err != nil {
return x, errors.Trace(err)
}
Expand Down