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

Jump to bottom

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

Merged
merged 11 commits into from
Aug 6, 2018
Merged

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fbbdde1
in non-strict SQL mode,fix column type is text /blob/json default val…
ciscoxll Aug 1, 2018
7a57b63
update comments
ciscoxll Aug 1, 2018
dbc1ac1
address comments
ciscoxll Aug 1, 2018
cc0babc
address comments
ciscoxll Aug 1, 2018
40aad3e
address comments
ciscoxll Aug 2, 2018
a3ab51a
Merge branch 'master' into sql-mode-compatible
ciscoxll Aug 2, 2018
eaa5850
address comments
ciscoxll Aug 3, 2018
69ae665
address comments
ciscoxll Aug 3, 2018
3b25e37
add testCheckColumnDefaultValue test
ciscoxll Aug 3, 2018
2226cb5
address comments
ciscoxll Aug 6, 2018
8916387
Merge branch 'master' into sql-mode-compatible
winkyao Aug 6, 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
54 changes: 54 additions & 0 deletions ddl/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2145,6 +2145,60 @@ func (s *testDBSuite) TestYearTypeCreateTable(c *C) {
c.Assert(mysql.HasUnsignedFlag(yearCol.Flag), IsFalse)
}

func (s *testDBSuite) TestCheckColumnDefaultValue(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("use test;")
s.tk.MustExec("drop table if exists text_default_text;")
s.testErrorCode(c, "create table text_default_text(c1 text not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_text(c1 text not null default 'scds');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("drop table if exists text_default_json;")
s.testErrorCode(c, "create table text_default_json(c1 json not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_json(c1 json not null default 'dfew555');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("drop table if exists text_default_blob;")
s.testErrorCode(c, "create table text_default_blob(c1 blob not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_blob(c1 blob not null default 'scds54');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("set sql_mode='';")
s.tk.MustExec("drop table if exists text_default_text;")
s.tk.MustExec("create table text_default_text(c1 text not null default '');")
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 about show create table text_default_text and show create table test_text_default_value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winkyao Done.

s.tk.MustQuery(`show create table text_default_text`).Check(testutil.RowsWithSep("|",
"text_default_text CREATE TABLE `text_default_text` (\n"+
" `c1` text NOT NULL\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))
ctx := s.tk.Se.(sessionctx.Context)
is := domain.GetDomain(ctx).InfoSchema()
tblInfo, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_text"))
c.Assert(err, IsNil)
c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, "")
Copy link
Contributor

@crazycs520 crazycs520 Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think the DefaultValue should be nil, not be ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultValue should be ""

Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the default value of the column is null, DefaultValue is "" .


s.tk.MustExec("drop table if exists text_default_blob;")
s.tk.MustExec("create table text_default_blob(c1 blob not null default '');")
s.tk.MustQuery(`show create table text_default_blob`).Check(testutil.RowsWithSep("|",
"text_default_blob CREATE TABLE `text_default_blob` (\n"+
" `c1` blob NOT NULL\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))
is = domain.GetDomain(ctx).InfoSchema()
tblInfo, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_blob"))
c.Assert(err, IsNil)
c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have explained.


s.tk.MustExec("drop table if exists text_default_json;")
s.tk.MustExec("create table text_default_json(c1 json not null default '');")
s.tk.MustQuery(`show create table text_default_json`).Check(testutil.RowsWithSep("|",
"text_default_json CREATE TABLE `text_default_json` (\n"+
" `c1` json NOT NULL DEFAULT 'null'\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))
is = domain.GetDomain(ctx).InfoSchema()
tblInfo, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_json"))
c.Assert(err, IsNil)
c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, `null`)
Copy link
Contributor

@crazycs520 crazycs520 Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ditto
I got it, It should be null....

}

func (s *testDBSuite) TestCharacterSetInColumns(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("drop database if exists varchar_test;")
Expand Down
34 changes: 24 additions & 10 deletions ddl/ddl_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,16 +251,32 @@ func buildColumnAndConstraint(ctx sessionctx.Context, offset int,
return col, cts, nil
}

// checkColumnCantHaveDefaultValue checks the column can have value as default or not.
// Now, TEXT/BLOB/JSON can't have not null value as default.
func checkColumnCantHaveDefaultValue(col *table.Column, value interface{}) (err error) {
// checkColumnDefaultValue checks the default value of the column.
// In non-strict SQL mode, if the default value of the column is an empty string, the default value can be ignored.
// In strict SQL mode, TEXT/BLOB/JSON can't have not null default values.
func checkColumnDefaultValue(ctx sessionctx.Context, col *table.Column, value interface{}) (bool, interface{}, error) {
hasDefaultValue := true
if value != nil && (col.Tp == mysql.TypeJSON ||
col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob ||
col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) {
// TEXT/BLOB/JSON can't have not null default values.
return errBlobCantHaveDefault.GenByArgs(col.Name.O)
// In non-strict SQL mode.
if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen when SQL mode change, create a table with SQLMode A and use it wit SQLMode B ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In non-strict SQL mode, if the default value of the column is an empty string, can ignore it, the table can be created successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand.

if col.Tp == mysql.TypeBlob || col.Tp == mysql.TypeLongBlob {
// The TEXT/BLOB default value can be ignored.
hasDefaultValue = false
}
// In non-strict SQL mode, if the column type is json and the default value is null, it is initialized to an empty array.
if col.Tp == mysql.TypeJSON {
value = `null`
}
sc := ctx.GetSessionVars().StmtCtx
sc.AppendWarning(errBlobCantHaveDefault.GenByArgs(col.Name.O))
return hasDefaultValue, value, nil
}
// In strict SQL mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

In strict SQL mode or default value is not an empty string.

return hasDefaultValue, value, errBlobCantHaveDefault.GenByArgs(col.Name.O)
}
return nil
return hasDefaultValue, value, nil
}

// isExplicitTimeStamp is used to check if explicit_defaults_for_timestamp is on or off.
Expand Down Expand Up @@ -326,11 +342,10 @@ func columnDefToCol(ctx sessionctx.Context, offset int, colDef *ast.ColumnDef, o
if err != nil {
return nil, nil, ErrColumnBadNull.Gen("invalid default value - %s", err)
}
if err = checkColumnCantHaveDefaultValue(col, value); err != nil {
if hasDefaultValue, value, err = checkColumnDefaultValue(ctx, col, value); err != nil {
return nil, nil, errors.Trace(err)
}
col.DefaultValue = value
hasDefaultValue = true
removeOnUpdateNowFlag(col)
case ast.ColumnOptionOnUpdate:
// TODO: Support other time functions.
Expand Down Expand Up @@ -1462,11 +1477,10 @@ func setDefaultAndComment(ctx sessionctx.Context, col *table.Column, options []*
if err != nil {
return ErrColumnBadNull.Gen("invalid default value - %s", err)
}
if err = checkColumnCantHaveDefaultValue(col, value); err != nil {
if hasDefaultValue, value, err = checkColumnDefaultValue(ctx, col, value); err != nil {
return errors.Trace(err)
}
col.DefaultValue = value
hasDefaultValue = true
case ast.ColumnOptionComment:
err := setColumnComment(ctx, col, opt)
if err != nil {
Expand Down