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: Support assign DEFAULT in ON DUPLICATE KEY UPDATE statement #13168

Merged
merged 19 commits into from
Nov 21, 2019
Merged

planner: Support assign DEFAULT in ON DUPLICATE KEY UPDATE statement #13168

merged 19 commits into from
Nov 21, 2019

Conversation

Deardrops
Copy link
Contributor

@Deardrops Deardrops commented Nov 5, 2019 •
edited
Loading

What problem does this PR solve?

Fix #13071

What is changed and how it works?

In statement insert into t value (1) on duplicate key update a=default, when we build the InsertPlan, we also check:

If the column need to be updated on duplicate key is generated column?

  • True: if the assigned variable is default?
    • True: we ignore this assignment.
    • False: throw the error The value specified for generated column 'gc' in table 't' is not allowed.
  • False: add the normal assignment to insertPlan.OnDuplicate

Check List

Tests

  • Integration test

Code changes

  • None

Side effects

  • Increased code complexity

Related changes

Release note

  • Support assign DEFAULT in ON DUPLICATE KEY UPDATE statement

@codecov
Copy link

codecov bot commented Nov 5, 2019 •
edited
Loading

Codecov Report

Merging #13168 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13168   +/-   ##
===========================================
  Coverage   80.0039%   80.0039%           
===========================================
  Files           473        473           
  Lines        116258     116258           
===========================================
  Hits          93011      93011           
  Misses        15944      15944           
  Partials       7303       7303

@Deardrops Deardrops changed the title planner: Support use on duplicate key update gc=default on generated columns planner: Support assign DEFAULT to generated column in on duplicate key update statement Nov 6, 2019
wjhuang2016
wjhuang2016 reviewed Nov 6, 2019
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

Please check insert into t1 values (1,default,default) on duplicate key update a=2, b=default(t);

djshow832
djshow832 reviewed Nov 6, 2019
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

@djshow832 djshow832 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 6, 2019
@Deardrops
Copy link
Contributor Author

/rebuild

@Deardrops
Copy link
Contributor Author

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

@Deardrops
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

@wjhuang2016, @djshow832, @zimulala, PTAL.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Nov 11, 2019

@wjhuang2016, @djshow832, @zimulala, PTAL.

@Deardrops Deardrops requested a review from a team as a code owner November 12, 2019 12:36
@ghost ghost requested review from wshwsh12 and XuHuaiyu November 12, 2019 12:36
@Deardrops
Copy link
Contributor Author

/run-all-tests

wjhuang2016
wjhuang2016 reviewed Nov 21, 2019
View reviewed changes
planner/core/planbuilder.go Outdated
}
// Note: For INSERT, REPLACE, and UPDATE, if a generated column is inserted into, replaced, or updated explicitly, the only permitted value is DEFAULT.
// see https://dev.mysql.com/doc/refman/8.0/en/create-table-generated-columns.html
if _, ok := generatedColumns[assign.Column.Name.L]; ok {
if isDefaultExpr {
if isDefault {
Copy link
Member

Choose a reason for hiding this comment

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

How about:
if defaultExpr := extractDefaultExpr(assign.Expr); defaultExpr != nil {
defaultExpr.Name = assign.Column expr.Name = assign.Column
} }
...
if defaultExpr != nil {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment addressed, PTAL @wjhuang2016

@sre-bot sre-bot mentioned this pull request Nov 21, 2019
Merged
@Deardrops
Copy link
Contributor Author

/run-all-tests

wjhuang2016
wjhuang2016 reviewed Nov 21, 2019
View reviewed changes
planner/core/planbuilder.go
isDefault = true
var defaultExpr *ast.DefaultExpr
if defaultExpr = extractDefaultExpr(assign.Expr); defaultExpr != nil {
defaultExpr.Name = assign.Column
Copy link
Member

Choose a reason for hiding this comment

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

defaultExpr := extractDefaultExpr(assign.Expr);
if defaultExpr != nil {
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wjhuang2016 Address comment, PTAL

@Deardrops
Copy link
Contributor Author

/run-all-tests

wjhuang2016
wjhuang2016 reviewed Nov 21, 2019
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@Deardrops Deardrops added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 21, 2019
@bb7133
Copy link
Member

bb7133 commented Nov 21, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

/run-all-tests

@sre-bot sre-bot merged commit 2759845 into pingcap:master Nov 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-3.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 21, 2019

cherry pick to release-2.1 failed

@Deardrops Deardrops mentioned this pull request Nov 22, 2019
Merged
sre-bot pushed a commit that referenced this pull request Dec 11, 2019
@Deardrops @sre-bot
planner: fixup some bugs with DEFAULT expression (#13168)(#13211)(#…
cba3e1a 
…12550)(#11901) (#13682)
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@Deardrops @XiaTianliang
planner: Support assign DEFAULT in ON DUPLICATE KEY UPDATE statem…
2c333de 
…ent (pingcap#13168)
@Deardrops Deardrops mentioned this pull request Feb 20, 2020
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

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

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@djshow832 djshow832 djshow832 left review comments

@bb7133 bb7133 bb7133 approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

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.

Support assign DEFAULT in ON DUPLICATE KEY UPDATE statement
8 participants
@Deardrops @sre-bot @bb7133 @zimulala @zz-jason @wjhuang2016 @djshow832 @coocood