executor: fix explicitly insert null value into timestamp column #3646
Conversation
create table t (ts timestamp); insert into t values (null); This should insert a null rather than default value.
|
LGTM |
| for i, v := range vals { | ||
| offset := cols[i].Offset | ||
| row[offset] = v | ||
| if !ignoreErr { |
There was a problem hiding this comment.
I'm also curious about this, could you explain it? @zimulala
There was a problem hiding this comment.
It's used for loading data, please keep it.
| @@ -134,7 +134,7 @@ func countEntriesWithPrefix(ctx context.Context, prefix []byte) (int, error) { | |||
| } | |||
|
|
|||
| func (ts *testSuite) TestTypes(c *C) { | |||
| _, err := ts.se.Execute("CREATE TABLE test.t (c1 tinyint, c2 smallint, c3 int, c4 bigint, c5 text, c6 blob, c7 varchar(64), c8 time, c9 timestamp not null default CURRENT_TIMESTAMP, c10 decimal(10,1))") | |||
| _, err := ts.se.Execute("CREATE TABLE test.t (c1 tinyint, c2 smallint, c3 int, c4 bigint, c5 text, c6 blob, c7 varchar(64), c8 time, c9 timestamp null default CURRENT_TIMESTAMP, c10 decimal(10,1))") | |||
There was a problem hiding this comment.
If table definition say the column can't be null, and we insert a null value, the operation should fail.
I just fix the test case make it behaves right.
There was a problem hiding this comment.
so the behavior of the cases which insert null values is wrong.
|
PTAL @zimulala |
| continue | ||
| for i, c := range e.Table.Cols() { | ||
| var needDefaultValue bool | ||
| if !hasValue[i] { |
There was a problem hiding this comment.
I think we can use row[i] == nil instead of this code, then we can remove this argument.
There was a problem hiding this comment.
No, we can't.
row[i] == nil is ambiguous.
create table t (a int, b int);
insert into t (a) values 1;
insert into t values (1, null);
Both case, row[1] == nil here, but they are different.
In the former case, b need a default value, the later case, b should be null.
executor/write.go
Outdated
| e.ctx.GetSessionVars().RetryInfo.AddAutoIncrementID(recordID) | ||
| } | ||
| } else { | ||
| needDefaultValue = false // handle it later. |
There was a problem hiding this comment.
Do we need to add a TODO here? Don't you want to handle what.
There was a problem hiding this comment.
Seems the comment is not accurate.
What I mean is if mysql.HasAutoIncrementFlag(c.Flag), row[i] don't needDefaultValue, it would be handled later, at line 915.
|
LGTM |
zimulala
added
status/LGT2
create table t (ts timestamp); insert into t values (null); This should insert a null rather than default value.
* expression, executor: fix hex(binary) (#3567) * expression, executor: fix unhex(binary) error (#3569) * expression: builtin now() should consider timestamp variable (#3590) * tidb/privilege/privileges: make show databases available with any global privilege (#3666) * privilege/privileges: skip privilege check for information_schema database (#3675) * conn: fix database info leaking problem (#3699) * server: escape database name "use `xxx`" in OpenCtx (#3713) * executor: fix explicitly insert null value into timestamp column (#3646) * util/types: fix parsing datatime 00-00-00 (#3536) * tidb-server: fix incorrect error message when auth failed (#3696)
get:
should get:
@zimulala @hanfei1991 @shenli @XuHuaiyu