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

planner: fixup some bugs with DEFAULT expression (#13168)(#13211)(#12550)(#11901) #13682

Merged
merged 12 commits into from
Dec 11, 2019
Merged

planner: fixup some bugs with DEFAULT expression (#13168)(#13211)(#12550)(#11901) #13682

merged 12 commits into from
Dec 11, 2019

Conversation

Deardrops
Copy link
Contributor

@Deardrops Deardrops commented Nov 22, 2019 •
edited
Loading

What problem does this PR solve?

Cherry-pick #13168 #13211 #12550 #11901

Related PR pingcap/parser#648

These are some bugs with DEFAULT expression:

  1. When insert t set c1 = default, TiDB will be panic. This bugs also occur in REPLACE / UPDATE / ON DUPLICATED UPDATE statement (planner: Fixup error when assign DEFAULT in INSERT/UPDATE/REPLACE ... SET ... statement #13211 planner: Support assign DEFAULT in ON DUPLICATE KEY UPDATE statement #13168). Meanwhile, in these statements, if column c1 is a generated column, MySQL allow these statements, but TiDB will throw an unexpected error (field c1 not found). (planner: fill missing origin table name for DEFAULT() function #12550)
  2. TiDB throw an unexpected error when insert DEFAULT value into generated columns. e.g. (planner: allow insert default into generated columns #11901)
create table t (a int default 1, b int generated always as (-a));
insert into t value (a, default); // TiDB error: Disallow assign value to generated column.

What is changed and how it works?

To solve 1st bugs, we split a huge PR into 3 tiny PR (#13211 #12550 #13168), for convenient of reviewing. Both of them are merged into master branch. After that, we cherry pick these 3 tiny PR in one PR.

Also, PR #11901 (the 2nd bug above) is related to DEFAULT expression, we cherry pick it in passing.

Conflicting files:

  • executor/insert_common.go
  • expression/expression.go
  • planner/core/logical_plan_builder.go
  • planner/core/planbuilder.go

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

  • Increased code complexity

Related changes

  • None

Release note

  • None

@Deardrops
Copy link
Contributor Author

/run-all-tests

@Deardrops
Copy link
Contributor Author

/run-all-tests

djshow832
djshow832 reviewed Nov 26, 2019
View reviewed changes
planner/core/logical_plan_builder.go
dbName = c.DBName
}
return colName, origColName, tblName, c.OrigTblName, c.DBName
return
Copy link
Contributor

Choose a reason for hiding this comment

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

This return can be removed.

Copy link
Contributor Author

@Deardrops Deardrops Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@djshow832 In Golang, if function has return values, it must explicit call return keyword. e.g

func f1() {
	fmt.Println("f1")
}
// print: f1

vs

func f1() (value int){
	fmt.Println("f2")
	value = 1
}
// compiler error: missing return at end of function

@djshow832
Copy link
Contributor

Any conflicting files?

@Deardrops Deardrops changed the title planner: fixup some bugs with DEFAULT expression planner: fixup some bugs with DEFAULT expression (#13168)(#13211)(#12550)(#11901) Nov 26, 2019
@djshow832
Copy link
Contributor

LGTM

@djshow832 djshow832 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 28, 2019

@djshow832, PTAL.

2 similar comments
@sre-bot
Copy link
Contributor

sre-bot commented Nov 30, 2019

@djshow832, PTAL.

@sre-bot
Copy link
Contributor

sre-bot commented Dec 3, 2019

@djshow832, PTAL.

@zz-jason
Copy link
Member

zz-jason commented Dec 3, 2019

@Deardrops It's better to cherry pick these PRs one by one.

zz-jason
zz-jason previously approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Dec 3, 2019
@Deardrops
Copy link
Contributor Author

@Deardrops It's better to cherry pick these PRs one by one.

@zz-jason Got it, I will pay attention to it in later PR.

@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2019

@djshow832, @zz-jason, PTAL.

@jackysp
Copy link
Member

jackysp commented Dec 6, 2019

Please resolve conflicts @Deardrops

@Deardrops
Copy link
Contributor Author

/run-all-tests

@jackysp
Copy link
Member

jackysp commented Dec 6, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 6, 2019

Your auto merge job has been accepted, waiting for 13784, 13306, 13737, 13938

@sre-bot
Copy link
Contributor

sre-bot commented Dec 6, 2019

/run-all-tests

@Deardrops
Copy link
Contributor Author

@jackysp all tests passed, PTAL

@sre-bot
Copy link
Contributor

sre-bot commented Dec 8, 2019

@djshow832, @zz-jason, PTAL.

@jackysp
Copy link
Member

jackysp commented Dec 9, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

@Deardrops merge failed.

@sre-bot
Copy link
Contributor

sre-bot commented Dec 11, 2019

@djshow832, @zz-jason, @jackysp, PTAL.

@jackysp
Copy link
Member

jackysp commented Dec 11, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 11, 2019

/run-all-tests

@sre-bot sre-bot merged commit cba3e1a into pingcap:release-3.0 Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djshow832 djshow832 djshow832 left review comments

@jackysp jackysp jackysp approved these changes

@zz-jason zz-jason zz-jason left review comments

Assignees
No one assigned
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Deardrops @djshow832 @sre-bot @zz-jason @jackysp