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

Fix bit default value bug #7249

Merged
merged 20 commits into from
Aug 31, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
af00e81
fix bit default value bug
crazycs520 Aug 2, 2018
d98444c
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 2, 2018
d8e001d
update test
crazycs520 Aug 2, 2018
21365d1
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 6, 2018
9190079
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 6, 2018
c1503eb
add comment
crazycs520 Aug 6, 2018
f2b8579
refine code
crazycs520 Aug 6, 2018
619da40
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 21, 2018
7ce59cd
address comment
crazycs520 Aug 23, 2018
5ac70c9
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 23, 2018
1462177
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 27, 2018
a4fd648
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 28, 2018
a5d4989
address comment
crazycs520 Aug 28, 2018
a330405
address comment
crazycs520 Aug 29, 2018
1e58ed9
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 29, 2018
5752ccb
Merge branch 'master' of https://github.com/pingcap/tidb into fix_bit…
crazycs520 Aug 30, 2018
c1747f6
Merge branch 'master' into fix_bit_default_value_bug
crazycs520 Aug 30, 2018
0b5d065
Merge branch 'master' into fix_bit_default_value_bug
ngaut Aug 30, 2018
5512cbf
Merge branch 'master' into fix_bit_default_value_bug
crazycs520 Aug 30, 2018
8c3bb8d
Merge branch 'master' into fix_bit_default_value_bug
crazycs520 Aug 31, 2018
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
9 changes: 9 additions & 0 deletions ddl/db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1576,6 +1576,15 @@ func (s *testDBSuite) TestCreateTable(c *C) {
c.Assert(err, NotNil)
}

func (s *testDBSuite) TestBitDefaultValue(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("create table t_bit (c1 bit(10) default 250, c2 int);")
tk.MustExec("insert into t_bit set c2=1;")
tk.MustQuery("select bin(c1),c2 from t_bit").Check(testkit.Rows("11111010 1"))
tk.MustExec("drop table t_bit")
}

func (s *testDBSuite) TestCreateTableWithPartition(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("use test;")
Expand Down
24 changes: 17 additions & 7 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,9 @@ func columnDefToCol(ctx sessionctx.Context, offset int, colDef *ast.ColumnDef, o
if hasDefaultValue, value, err = checkColumnDefaultValue(ctx, col, value); err != nil {
return nil, nil, errors.Trace(err)
}
col.DefaultValue = value
if err = col.SetDefaultValue(value); err != nil {
return nil, nil, errors.Trace(err)
}
removeOnUpdateNowFlag(col)
case ast.ColumnOptionOnUpdate:
// TODO: Support other time functions.
Expand Down Expand Up @@ -500,7 +502,7 @@ func checkDefaultValue(ctx sessionctx.Context, c *table.Column, hasDefaultValue
return nil
}

if c.DefaultValue != nil {
if c.GetDefaultValue() != nil {
if _, err := table.GetColDefaultValue(ctx, c.ToInfo()); err != nil {
return types.ErrInvalidDefault.GenByArgs(c.Name)
}
Expand All @@ -522,7 +524,7 @@ func checkDefaultValue(ctx sessionctx.Context, c *table.Column, hasDefaultValue
// checkPriKeyConstraint check all parts of a PRIMARY KEY must be NOT NULL
func checkPriKeyConstraint(col *table.Column, hasDefaultValue, hasNullFlag bool, outPriKeyConstraint *ast.Constraint) error {
// Primary key should not be null.
if mysql.HasPriKeyFlag(col.Flag) && hasDefaultValue && col.DefaultValue == nil {
if mysql.HasPriKeyFlag(col.Flag) && hasDefaultValue && col.GetDefaultValue() == nil {
return types.ErrInvalidDefault.GenByArgs(col.Name)
}
// Set primary key flag for outer primary key constraint.
Expand Down Expand Up @@ -1247,7 +1249,7 @@ func (d *ddl) AddColumn(ctx sessionctx.Context, ti ast.Ident, spec *ast.AlterTab
if err != nil {
return errors.Trace(err)
}
col.OriginDefaultValue = col.DefaultValue
col.OriginDefaultValue = col.GetDefaultValue()
if col.OriginDefaultValue == nil && mysql.HasNotNullFlag(col.Flag) {
zeroVal := table.GetZeroValue(col.ToInfo())
col.OriginDefaultValue, err = zeroVal.ToString()
Expand Down Expand Up @@ -1458,7 +1460,10 @@ func setDefaultValue(ctx sessionctx.Context, col *table.Column, option *ast.Colu
if err != nil {
return ErrColumnBadNull.Gen("invalid default value - %s", err)
}
col.DefaultValue = value
err = col.SetDefaultValue(value)
if err != nil {
return errors.Trace(err)
}
return errors.Trace(checkDefaultValue(ctx, col, true))
}

Expand Down Expand Up @@ -1487,7 +1492,9 @@ func setDefaultAndComment(ctx sessionctx.Context, col *table.Column, options []*
if hasDefaultValue, value, err = checkColumnDefaultValue(ctx, col, value); err != nil {
return errors.Trace(err)
}
col.DefaultValue = value
if err = col.SetDefaultValue(value); err != nil {
return errors.Trace(err)
}
case ast.ColumnOptionComment:
err := setColumnComment(ctx, col, opt)
if err != nil {
Expand Down Expand Up @@ -1709,7 +1716,10 @@ func (d *ddl) AlterColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Alt
// Clean the NoDefaultValueFlag value.
col.Flag &= ^mysql.NoDefaultValueFlag
if len(specNewColumn.Options) == 0 {
col.DefaultValue = nil
err = col.SetDefaultValue(nil)
if err != nil {
return errors.Trace(err)
}
setNoDefaultValueFlag(col, false)
} else {
err = setDefaultValue(ctx, col, specNewColumn.Options[0])
Expand Down
5 changes: 3 additions & 2 deletions executor/show.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,8 @@ func (e *ShowExec) fetchShowCreateTable() error {
buf.WriteString(" NOT NULL")
}
if !mysql.HasNoDefaultValueFlag(col.Flag) {
switch col.DefaultValue {
defaultValue := col.GetDefaultValue()
switch defaultValue {
case nil:
if !mysql.HasNotNullFlag(col.Flag) {
if col.Tp == mysql.TypeTimestamp {
Expand All @@ -514,7 +515,7 @@ func (e *ShowExec) fetchShowCreateTable() error {
case "CURRENT_TIMESTAMP":
buf.WriteString(" DEFAULT CURRENT_TIMESTAMP")
default:
defaultValStr := fmt.Sprintf("%v", col.DefaultValue)
defaultValStr := fmt.Sprintf("%v", defaultValue)
if col.Tp == mysql.TypeBit {
defaultValBinaryLiteral := types.BinaryLiteral(defaultValStr)
buf.WriteString(fmt.Sprintf(" DEFAULT %s", defaultValBinaryLiteral.ToBitLiteralString(true)))
Expand Down
22 changes: 22 additions & 0 deletions model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type ColumnInfo struct {
Offset int `json:"offset"`
OriginDefaultValue interface{} `json:"origin_default"`
DefaultValue interface{} `json:"default"`
DefaultValueBit []byte `json:"default_bit"`
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use a uniform default value in cloumn info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to write down how to do it in the description of PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What's the problem
  2. How to do it
  3. Is there any compatibility problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciscoxll done. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be incompatible when rolling update TiDB?
As follows case:
On new TiDB, we builds a "add column" job , then we handle the DDL job on Old TiDB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@crazycs520 crazycs520 Aug 22, 2018

Choose a reason for hiding this comment

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

@zimulala No. It is compatible.
if old tidb to handle job that created by new tidb, old tidb will ignore the DefaultValueBit .
I have done an experiment in this scene.

GeneratedExprString string `json:"generated_expr_string"`
GeneratedStored bool `json:"generated_stored"`
Dependences map[string]struct{} `json:"dependences"`
Expand All @@ -87,6 +88,27 @@ func (c *ColumnInfo) IsGenerated() bool {
return len(c.GeneratedExprString) != 0
}

// SetDefaultValue sets the default value.
func (c *ColumnInfo) SetDefaultValue(value interface{}) error {
c.DefaultValue = value
if c.Tp == mysql.TypeBit {
if v, ok := value.(string); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add more description here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

c.DefaultValueBit = []byte(v)
return nil
}
return types.ErrInvalidDefault.GenByArgs(c.Name)
}
return nil
}

// GetDefaultValue gets the default value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we need to add more comments. I understand that GetDefaultValue will return the default value of the column but I am more interested in the case handling logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetDefaultValue just return the column's default value.
just use column.GetDefaultValue() instead of column.DefaultValue,
because the bit type default value will stored in DefaultValueBit field.

func (c *ColumnInfo) GetDefaultValue() interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change all "c.DefaultValue" to "GetDefaultValue()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ye~ I have already changed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"if column.DefaultValue != nil" in column.go, the ColumnInfo in column.go is not the ColumnInfo in model.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to replace "c.DefaultValue = types.ZeroDatetimeStr" with "SetDefaultValue()" in ddl_api.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh...
ye...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

if c.Tp == mysql.TypeBit && c.DefaultValueBit != nil {
return string(c.DefaultValueBit)
Copy link
Member

Choose a reason for hiding this comment

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

should we use hack.String()?

}
return c.DefaultValue
}

// FindColumnInfo finds ColumnInfo in cols by name.
func FindColumnInfo(cols []*ColumnInfo, name string) *ColumnInfo {
name = strings.ToLower(name)
Expand Down
4 changes: 2 additions & 2 deletions table/column.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func NewColDesc(col *Column) *ColDesc {
}
var defaultValue interface{}
if !mysql.HasNoDefaultValueFlag(col.Flag) {
defaultValue = col.DefaultValue
defaultValue = col.GetDefaultValue()
}

extra := ""
Expand Down Expand Up @@ -310,7 +310,7 @@ func GetColOriginDefaultValue(ctx sessionctx.Context, col *model.ColumnInfo) (ty

// GetColDefaultValue gets default value of the column.
func GetColDefaultValue(ctx sessionctx.Context, col *model.ColumnInfo) (types.Datum, error) {
return getColDefaultValue(ctx, col, col.DefaultValue)
return getColDefaultValue(ctx, col, col.GetDefaultValue())
}

func getColDefaultValue(ctx sessionctx.Context, col *model.ColumnInfo, defaultVal interface{}) (types.Datum, error) {
Expand Down
2 changes: 1 addition & 1 deletion table/tables/tables.go
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ func CanSkip(info *model.TableInfo, col *table.Column, value types.Datum) bool {
if col.IsPKHandleColumn(info) {
return true
}
if col.DefaultValue == nil && value.IsNull() {
if col.GetDefaultValue() == nil && value.IsNull() {
return true
}
if col.IsGenerated() && !col.GeneratedStored {
Expand Down