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

*: make insert with calculated value behave the same as MySQL. #4603

Merged
merged 16 commits into from Sep 27, 2017

Conversation

Projects
None yet
7 participants
@bobotu
Contributor

bobotu commented Sep 22, 2017

This PR try to fix most cases of #4488.
Now insert can correctly handle calculated value based on other columns,
such as insert into t1 set b=a+1 and insert into t1 (b) value (a).
Last, as mentioned in issue, make the error message more readable.

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 22, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot commented Sep 22, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 22, 2017

Contributor

Can the sql in #4482 be fixed after this commit when there is no default value for columns?

Contributor

XuHuaiyu commented Sep 22, 2017

Can the sql in #4482 be fixed after this commit when there is no default value for columns?

@bobotu

This comment has been minimized.

Show comment
Hide comment
@bobotu

bobotu Sep 22, 2017

Contributor

@XuHuaiyu It can be fixed, but need a little change in fillDefaultValues. I'll file a new commit soon.

Contributor

bobotu commented Sep 22, 2017

@XuHuaiyu It can be fixed, but need a little change in fillDefaultValues. I'll file a new commit soon.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 22, 2017

Contributor

PTAL @winkyao

Contributor

XuHuaiyu commented Sep 22, 2017

PTAL @winkyao

Show outdated Hide outdated executor/write_test.go
Show outdated Hide outdated executor/write_test.go
@winoros

GetZeroValue didn't cover the JSON case.
You can use Datum.SetMysqlJson(json.CreateJson(nil))

bobotu added some commits Sep 22, 2017

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Sep 24, 2017

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 25, 2017

Member

@bobotu any update?

Member

winkyao commented Sep 25, 2017

@bobotu any update?

@bobotu

This comment has been minimized.

Show comment
Hide comment
@bobotu

bobotu Sep 25, 2017

Contributor

@winkyao I have added a bundle of test case as mentioned above.

Contributor

bobotu commented Sep 25, 2017

@winkyao I have added a bundle of test case as mentioned above.

Show outdated Hide outdated executor/write.go
@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Sep 26, 2017

@winoros PTAL

Show outdated Hide outdated plan/planbuilder.go
@@ -801,8 +803,6 @@ func (b *planBuilder) buildInsert(insert *ast.InsertStmt) Plan {
b.err = ErrBadGeneratedColumn.GenByArgs(assign.Column.Name.O, tableInfo.Name.O)
return nil
}
// Here we keep different behaviours with MySQL. MySQL allow set a = b, b = a and the result is NULL, NULL.

This comment has been minimized.

@winkyao

winkyao Sep 26, 2017

Member

keep the comment

@winkyao

winkyao Sep 26, 2017

Member

keep the comment

This comment has been minimized.

@bobotu

bobotu Sep 26, 2017

Contributor

But after merge this PR, result of set a = b, b = a is NULL, NULL.

@bobotu

bobotu Sep 26, 2017

Contributor

But after merge this PR, result of set a = b, b = a is NULL, NULL.

This comment has been minimized.

@winkyao

winkyao Sep 26, 2017

Member

ok. got it.

@winkyao

winkyao Sep 26, 2017

Member

ok. got it.

Show outdated Hide outdated table/column.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated table/column.go
Show outdated Hide outdated table/column.go
Show outdated Hide outdated plan/planbuilder.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
@@ -1171,3 +1171,106 @@ func (s *testSuite) TestIssue4067(c *C) {
tk.MustExec("delete from t1 where id in (select id from t2)")
tk.MustQuery("select * from t1").Check(nil)
}
func (s *testSuite) TestInsertCalculatedValue(c *C) {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 26, 2017

Contributor

add test cases for more generated columns.

@XuHuaiyu

XuHuaiyu Sep 26, 2017

Contributor

add test cases for more generated columns.

Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go

bobotu added some commits Sep 27, 2017

@bobotu

This comment has been minimized.

Show comment
Hide comment
@bobotu

bobotu Sep 27, 2017

Contributor

@winkyao PTAL

Contributor

bobotu commented Sep 27, 2017

@winkyao PTAL

@winkyao winkyao removed the status/LGT1 label Sep 27, 2017

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 27, 2017

Member

/ok-to-test

Member

winkyao commented Sep 27, 2017

/ok-to-test

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 27, 2017

Member

/run-all-test

Member

winkyao commented Sep 27, 2017

/run-all-test

Show outdated Hide outdated executor/write.go
Show outdated Hide outdated executor/write.go
@@ -980,6 +1028,9 @@ func (e *InsertValues) initDefaultValues(row []types.Datum, hasValue []bool, ign
// Just leave generated column as null. It will be calculated later

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 27, 2017

Contributor

Should we add a comment for initDefaultValues
to distinguish it with fillDefaultValues easily.

@XuHuaiyu

XuHuaiyu Sep 27, 2017

Contributor

Should we add a comment for initDefaultValues
to distinguish it with fillDefaultValues easily.

@@ -980,6 +1028,9 @@ func (e *InsertValues) initDefaultValues(row []types.Datum, hasValue []bool, ign
// Just leave generated column as null. It will be calculated later
// but before we check whether the column can be null or not.
needDefaultValue = false
if !hasValue[i] {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 27, 2017

Contributor

any test case for this check?

@XuHuaiyu

XuHuaiyu Sep 27, 2017

Contributor

any test case for this check?

This comment has been minimized.

@bobotu

bobotu Sep 27, 2017

Contributor

Test L1279-L1281:

create table t(a int auto_increment key, b int);
set SQL_MODE=NO_AUTO_VALUE_ON_ZERO;
insert into t (b) value (a+1);

If we don't reset a to NULL, a will be 0.

@bobotu

bobotu Sep 27, 2017

Contributor

Test L1279-L1281:

create table t(a int auto_increment key, b int);
set SQL_MODE=NO_AUTO_VALUE_ON_ZERO;
insert into t (b) value (a+1);

If we don't reset a to NULL, a will be 0.

reset

@winkyao winkyao added status/LGT1 and removed status/LGT2 labels Sep 27, 2017

bobotu and others added some commits Sep 27, 2017

Show outdated Hide outdated executor/write.go
@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 27, 2017

Member

/run-all-test

Member

winkyao commented Sep 27, 2017

/run-all-test

bobotu and others added some commits Sep 27, 2017

@winkyao

LGTM

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 27, 2017

Member

/run-all-test

Member

winkyao commented Sep 27, 2017

/run-all-test

@hanfei1991 hanfei1991 merged commit a6756a4 into pingcap:master Sep 27, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment