plan: refactor the code of building `Insert`. #7068

winoros · 2018-07-17T08:08:16Z

What have you changed? (mandatory)

Refactor the code when building Insert plan.
To fix some bugs make code more maintainable.
Fix #7064
Fix #7061
And another bug:
the table have 3 columns.
insert into t values(default, default, default, default) will make tidb panicked.

Main change:
We used to check the length of value_list and column_list when executing. Now we move this change to planner.

What is the type of the changes? (mandatory)

Refactor
Improvement
Bug fix

How has this PR been tested? (mandatory)

existing tests and newly added tests.

Does this PR need to be added to the release notes? (mandatory)

Fix the behavior of INSERT statement in some scenarios.

jackysp · 2018-07-17T09:36:49Z

Please fix CI @winoros

winoros · 2018-07-17T11:00:34Z

/run-all-tests

winoros · 2018-07-17T11:26:21Z

/run-integration-common-test tidb-test=pr/589

winoros · 2018-07-17T11:34:20Z

/run-integration-ddl-test tidb-test=pr/589

winoros · 2018-07-17T11:50:49Z

Integration-common-test is failed because the bug in tikv.
Now it's ready to review.

zz-jason · 2018-07-18T08:11:59Z

plan/logical_plan_test.go

@@ -1332,7 +1332,7 @@ func (s *testPlanSuite) TestVisitInfo(c *C) {
 		ans []visitInfo
 	}{
 		{
-			sql: "insert into t values (1)",


Does the old test result in an error after this change?

Yes, this the length of value_list is not same with the number of table's columns.

zz-jason · 2018-07-19T02:30:09Z

plan/planbuilder.go

+	return onDupColSet, dupCols, nil
+}
+
+func (p *Insert) getAffectCols(insert *ast.InsertStmt) (affectedValuesCols []*table.Column, err error) {


I think it's better to make the receiver of this function be the planBuilder.

how about s/getAffectCols/getInsertColumns/?

zz-jason · 2018-07-19T02:57:46Z

plan/planbuilder.go

+		}
+
+	} else if len(insert.Setlist) == 0 {
+		// Branch for `INSERT INTO tbl_name {VALUES | VALUE} (value_list) [, (value_list)] ...`,


How about:

// This is for the following scenario: // 1. `INSERT INTO tbl_name {VALUES | VALUE} ...` // 2. `INSERT INTO tbl_name SELECT ...`

zz-jason · 2018-07-19T03:01:36Z

plan/planbuilder.go

+
+func (p *Insert) getAffectCols(insert *ast.InsertStmt) (affectedValuesCols []*table.Column, err error) {
+	if len(insert.Columns) > 0 {
+		// Branch for `INSERT INTO tbl_name (col_name [, col_name] ...) {VALUES | VALUE} (value_list) [, (value_list)] ...`,


How about:

// This is for the following scenario: // 1. `INSERT INTO tbl_name (col_name [, col_name] ...) {VALUES | VALUE} ...` // 2. `INSERT INTO tbl_name (col_name [, col_name] ...) SELECT ...`

zz-jason · 2018-07-19T03:33:53Z

plan/planbuilder.go

 			}
 		}
+		// If the value_list and col_list is empty and we have generated column, we can still write to this table.


This comment should reside to if len(insert.Columns) == 0 && len(insert.Lists[0]) == 0 branch? Maybe we can remove it?

Yes, it's for the else branch, explaining that else branch doesn't need to do anything.
I think it's useful. But maybe we can move it other place.

I think we can put this comment to the if len(insert.Columns) > 0 || len(insert.Lists[0]) > 0 { to explain why we don't check whether there is generated column in the table when the value_list and col_list is empty.

zz-jason · 2018-07-19T03:41:19Z

plan/planbuilder.go

+		// "insert into t values (), (1)" is not valid.
+		// "insert into t values (1), ()" is not valid.
+		// "insert into t values (1,2), (1)" is not valid.
+		if i > 0 && len(insert.Lists[i-1]) != len(valuesItem) {


Maybe this form can help reading the code:

if i > 0 && len(insert.Lists[i]) != len(insert.Lists[i-1])

zz-jason · 2018-07-19T03:43:33Z

plan/planbuilder.go

+		for j, valueItem := range valuesItem {
+			var expr expression.Expression
+			var err error
+			if dft, ok := valueItem.(*ast.DefaultExpr); ok {


Usually the abbreviation for default is def, see https://www.abbreviations.com/abbreviation/Default

zz-jason · 2018-07-19T03:45:52Z

plan/planbuilder.go

+		for j, valueItem := range valuesItem {
+			var expr expression.Expression
+			var err error
+			if dft, ok := valueItem.(*ast.DefaultExpr); ok {


I think the "type switch" is more suitable.

zz-jason · 2018-07-19T03:47:47Z

plan/planbuilder.go

-			return nil
+	totalTableCols := insertPlan.Table.Cols()
+	for i, valuesItem := range insert.Lists {
+		// The length of the value_list should keep unchanged.


How about:

// The lengths of all the value lists should be the same, e.g:

zz-jason · 2018-07-20T01:31:04Z

plan/planbuilder.go

-		if err != nil {
-			b.err = errors.Trace(err)
-			return nil
+	// Check that there's no generated column.


@CaitinChen @lilin90 PTAL this comment, thanks

Check to guarantee that no generated column exists.

jackysp

LGTM

…into insert-plan-refactor

winoros · 2018-07-24T08:00:07Z

PTAL @zz-jason

zz-jason · 2018-07-24T08:12:28Z

plan/planbuilder.go

+	// i.e. table t have two column a and b where b is generated column.
+	// `insert into t (b) select * from t` will raise a error which tells you that the column count is not matched.
+	// `insert into t select * from t` will raise a error which tells you that there's generated column in column list.
+	// If we do this check before the above one, the first example will raise the error same with the second example.


@CaitinChen Could help us to refine this comment block 😂

If we do this check before the above one, "insert into t (b) select * from t" will raise an error that there's a generated column in the column list.
Note: I don't use "the first example" and "the second example" because there is only one example. "insert into t (b) select * from t" and "insert into t select * from t" are not named as two examples. Both of them are in this example.

CaitinChen · 2018-07-24T09:16:18Z

plan/planbuilder.go

-			return nil
+	// Check to guarantee that there's no generated column.
+	// This check is done after the above one is to make compatible with mysql.
+	// i.e. table t have two column a and b where b is generated column.


For example, table t has two columns, namely a and b, and b is a generated column.

Note: "i.e." means "that is to say" or "namely".

CaitinChen · 2018-07-24T09:19:45Z

plan/planbuilder.go

+	// This check is done after the above one is to make compatible with mysql.
+	// i.e. table t have two column a and b where b is generated column.
+	// `insert into t (b) select * from t` will raise a error which tells you that the column count is not matched.
+	// `insert into t select * from t` will raise a error which tells you that there's generated column in column list.


"insert into t select * from t" will raise an error that there's a generated column in the column list.

CaitinChen · 2018-07-24T09:33:08Z

plan/planbuilder.go

+	// Check to guarantee that there's no generated column.
+	// This check is done after the above one is to make compatible with mysql.
+	// i.e. table t have two column a and b where b is generated column.
+	// `insert into t (b) select * from t` will raise a error which tells you that the column count is not matched.


"insert into t (b) select * from t" will raise an error that the column count is not matched.

Note: I use "insert into t (b) select * from t" instead of insert into t (b) select * from t because the inline code format will not be displayed in the code comments as expected.

CaitinChen · 2018-07-24T09:40:18Z

plan/planbuilder.go

+	// i.e. table t have two column a and b where b is generated column.
+	// `insert into t (b) select * from t` will raise a error which tells you that the column count is not matched.
+	// `insert into t select * from t` will raise a error which tells you that there's generated column in column list.
+	// If we do this check before the above one, the first example will raise the error same with the second example.


If we do this check before the above one, "insert into t (b) select * from t" will raise an error that there's a generated column in the column list.
Note: I don't use "the first example" and "the second example" because there is only one example. "insert into t (b) select * from t" and "insert into t select * from t" are not named as two examples. Both of them are in this example.

CaitinChen · 2018-07-24T10:08:08Z

plan/planbuilder.go

-			b.err = errors.Trace(err)
-			return nil
+	// Check to guarantee that there's no generated column.
+	// This check is done after the above one is to make compatible with mysql.


This check should be done after the above one to make its behavior compatible with MySQL.

zz-jason

LGTM

winoros · 2018-07-25T11:54:20Z

@jackysp PTAL

jackysp

LGTM

jackysp · 2018-07-25T13:53:50Z

/run-all-tests

winoros · 2018-07-25T14:26:28Z

/run-integration-ddl-test tidb-test=pr/589

plan: refactor the code of building Insert.

78a3660

winoros added type/bugfix This PR fixes a bug. sig/planner SIG: Planner release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 17, 2018

winoros requested review from jackysp, coocood and zz-jason July 17, 2018 08:08

fix test.

8d99c66

winoros force-pushed the insert-plan-refactor branch from 2e963da to 8d99c66 Compare July 17, 2018 10:56

Merge branch 'master' into insert-plan-refactor

045f57a

zz-jason reviewed Jul 19, 2018

View reviewed changes

zz-jason reviewed Jul 20, 2018

View reviewed changes

Merge branch 'master' into insert-plan-refactor

4d73d6f

jackysp reviewed Jul 23, 2018

View reviewed changes

winoros added 2 commits July 24, 2018 15:56

address comments.

fd045ea

Merge branch 'insert-plan-refactor' of https://github.com/winoros/tidb …

d9d0fa0

…into insert-plan-refactor

zz-jason reviewed Jul 24, 2018

View reviewed changes

jackysp previously approved these changes Jul 24, 2018

View reviewed changes

CaitinChen reviewed Jul 24, 2018

View reviewed changes

address comments.

312010e

winoros dismissed jackysp’s stale review via 312010e July 25, 2018 08:08

address comment.

429dc51

zz-jason approved these changes Jul 25, 2018

View reviewed changes

jackysp approved these changes Jul 25, 2018

View reviewed changes

Merge branch 'master' into insert-plan-refactor

d2a3bbd

jackysp added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 25, 2018

winoros merged commit b5f9b35 into pingcap:master Jul 25, 2018

winoros deleted the insert-plan-refactor branch July 25, 2018 14:41

CaitinChen mentioned this pull request Aug 1, 2018

plan: update comment wording #7238

Merged

plan: refactor the code of building Insert. #7068

plan: refactor the code of building Insert. #7068

Conversation

winoros commented Jul 17, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

jackysp commented Jul 17, 2018

winoros commented Jul 17, 2018

winoros commented Jul 17, 2018

winoros commented Jul 17, 2018

winoros commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaitinChen Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

winoros commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaitinChen Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

winoros commented Jul 25, 2018

jackysp left a comment

Choose a reason for hiding this comment

jackysp commented Jul 25, 2018

winoros commented Jul 25, 2018

plan: refactor the code of building `Insert`. #7068

plan: refactor the code of building `Insert`. #7068

winoros commented Jul 17, 2018 •

edited

Loading

CaitinChen Jul 20, 2018 •

edited

Loading

CaitinChen Jul 24, 2018 •

edited

Loading