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

Jump to bottom

executor: remove some useless code and avoid some redundancy check #7639

Merged
merged 16 commits into from Oct 12, 2018
Merged

executor: remove some useless code and avoid some redundancy check #7639

merged 16 commits into from Oct 12, 2018

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Sep 7, 2018 •
edited

What problem does this PR solve?

Remove some useless code and avoid some redundancy check.

What is changed and how it works?

Remove finished in all DMLs. Avoid once null check when filling the rows.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

PTAL @coocood

Merge it with https://github.com/pingcap/tidb-test/pull/623

@jackysp
Copy link
Member Author

jackysp commented Sep 7, 2018

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Sep 11, 2018

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Sep 11, 2018

/run-all-tests tidb-test=pr/623

@jackysp
Copy link
Member Author

jackysp commented Sep 13, 2018

PTAL @coocood

tiancaiamao
tiancaiamao reviewed Sep 17, 2018
View reviewed changes
table/column_test.go
@@ -338,7 +338,7 @@ func (t *testTableSuite) TestGetDefaultValue(c *C) {
},
},
true,
types.Datum{},
types.NewIntDatum(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it a unit test for function GetColDefaultValue. It is changed due to the behavior of GetColDefaultValue was changed.

table/column.go Outdated
@@ -339,7 +353,7 @@ func getColDefaultValue(ctx sessionctx.Context, col *model.ColumnInfo, defaultVa

func getColDefaultValueFromNil(ctx sessionctx.Context, col *model.ColumnInfo) (types.Datum, error) {
if !mysql.HasNotNullFlag(col.Flag) {
return types.Datum{}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

types.Datum{} is better than types.NewDatum(nil)

table/column.go Outdated
return GetZeroValue(col), nil
}
if col.IsGenerated() {
return types.NewDatum(nil), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the generated column have a not null flag ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be checked in HandleBadNull.

executor/insert_common.go
// For NOT NULL column, it will return error or use zero value based on sql_mode.
func (e *InsertValues) initDefaultValues(row []types.Datum, hasValue []bool) error {
func (e *InsertValues) fillRow(cols []*table.Column, row []types.Datum, hasValue []bool, valLen int) ([]types.Datum, error) {
for i, c := range e.Table.Cols() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are cols from caller and e.Table.Cols(), which one should we use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I think it may contain some bugs.

executor/insert_common.go Outdated

// Evaluate the generated columns.
if c.IsGenerated() {
for i, expr := range e.GenExprs {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/i/j

executor/insert_common.go Outdated
return types.Datum{}, errors.Trace(err)
}
return d, nil
} else if !hasValue {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/else if/ if

Copy link
Contributor

Choose a reason for hiding this comment

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

The "not auto increment" && "not has value case".

Copy link
Member Author

Choose a reason for hiding this comment

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

If not auto increment and not has value, we could return the datum directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, when "not auto increment" && "not has value case", the returned value is NULL, rather then a column default value, even the column has default value defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

else if can be changed to if, rest LGTM

@tiancaiamao tiancaiamao added the require-LGT3 Indicates that the PR requires three LGTM. label Sep 17, 2018
@jackysp
Copy link
Member Author

jackysp commented Sep 21, 2018

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

@zz-jason
Copy link
Member

LGTM
@jackysp Please resolve the conflicts.

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 27, 2018
@jackysp
Copy link
Member Author

jackysp commented Sep 27, 2018

PTAL @tiancaiamao

@jackysp jackysp added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 27, 2018
@jackysp
Copy link
Member Author

jackysp commented Sep 27, 2018

PTAL @coocood

@tiancaiamao
Copy link
Contributor

LGTM

@jackysp
Copy link
Member Author

jackysp commented Sep 28, 2018

PTAL @coocood

@jackysp
Copy link
Member Author

jackysp commented Sep 29, 2018

PTAL @coocood

1 similar comment
@jackysp
Copy link
Member Author

jackysp commented Oct 8, 2018

PTAL @coocood

@jackysp jackysp added the type/bug-fix This PR fixes a bug. label Oct 9, 2018
@jackysp
Copy link
Member Author

jackysp commented Oct 10, 2018

PTAL @lysu

lysu
lysu reviewed Oct 10, 2018
View reviewed changes
executor/insert_common.go Show resolved Hide resolved
executor/insert_common.go Show resolved Hide resolved
executor/insert_common.go Show resolved Hide resolved
executor/write_test.go Show resolved Hide resolved
executor/insert_common.go Outdated Show resolved Hide resolved
executor/insert_common.go Show resolved Hide resolved
@jackysp
Copy link
Member Author

jackysp commented Oct 11, 2018

PTAL @lysu

lysu
lysu approved these changes Oct 11, 2018
View reviewed changes
Copy link
Collaborator

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu
Copy link
Collaborator

lysu commented Oct 11, 2018

/run-all-tests

@lysu lysu added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 11, 2018
@zz-jason
Copy link
Member

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Oct 12, 2018

/run-all-tests tidb-test=pr/623

1 similar comment
@jackysp
Copy link
Member Author

jackysp commented Oct 12, 2018

/run-all-tests tidb-test=pr/623

@jackysp jackysp merged commit d7a59ec into pingcap:master Oct 12, 2018
@jackysp jackysp deleted the refactor_insert branch October 12, 2018 04:58
jackysp added a commit to jackysp/tidb that referenced this pull request Oct 12, 2018
@jackysp
executor: remove some useless code and avoid some redundancy check (p…
a69b0f4 
…ingcap#7639)
@jackysp jackysp mentioned this pull request Oct 12, 2018
Merged
ngaut pushed a commit that referenced this pull request Oct 15, 2018
@jackysp @ngaut
executor: remove some useless code and avoid some redundancy check (#…
a84cce1 
…7639) (#7881)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao left review comments

@lysu lysu lysu approved these changes

Assignees
No one assigned
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution status/LGT3 The PR has already had 3 LGTM. type/bug-fix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jackysp @zz-jason @tiancaiamao @lysu