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

issues 280: Support ON UPDATE ddl statements #331

Merged
merged 2 commits into from
Jul 20, 2019

Conversation

gleonid
Copy link
Contributor

@gleonid gleonid commented May 20, 2019

What problem does this PR solve?

"CREATE TABLE child ( id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id) ON UPDATE CASCADE)" fails to parse even though grammar looks like it should b able to

What is changed and how it works?

  1. Added a unit test for above (see changes to ast/ddl_test.go)
  2. refactored slightly grammar definition to explicitly enumerate all 4 possible combinations:
    ReferDef: "REFERENCES" TableName '(' IndexColNameList ')' OnDeleteOpt OnUpdateOpt is replaced with
 ReferDef:
       "REFERENCES" TableName '(' IndexColNameList ')' OnDeleteUpdateOpt
...
OnDeleteUpdateOpt:
    OnDelete OnUpdate
    {
        $$ = OnDeleteUpdateDef{ $1.(*ast.OnDeleteOpt), $2.(*ast.OnUpdateOpt) }
       }
|   OnUpdate
    {
        $$ = OnDeleteUpdateDef{ &ast.OnDeleteOpt{}, $1.(*ast.OnUpdateOpt) }
    } %prec lowerThanOn
|   OnDelete
    {
        $$ = OnDeleteUpdateDef{ $1.(*ast.OnDeleteOpt), &ast.OnUpdateOpt{} }
    } %prec lowerThanOn
|   {
               $$ = OnDeleteUpdateDef{ &ast.OnDeleteOpt{}, &ast.OnUpdateOpt{} }
       } %prec lowerThanOn

Check List

Tests

  • Unit test
    added

Code changes

  • Has exported function/method change
    no
  • Has exported variable/fields change
    no
  • Has interface methods change
    no

Side effects

  • Possible performance regression
    performance test was not performed
  • Breaking backward compatibility
    existing tests pass

@CLAassistant
Copy link

CLAassistant commented May 20, 2019

CLA assistant check
All committers have signed the CLA.

@codecov

This comment has been minimized.

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #331   +/-   ##
=======================================
  Coverage   53.38%   53.38%           
=======================================
  Files          31       31           
  Lines        6519     6519           
=======================================
  Hits         3480     3480           
  Misses       2700     2700           
  Partials      339      339
Impacted Files Coverage Δ
parser.go 70.58% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8395edb...5ead50a. Read the comment docs.

@gleonid
Copy link
Contributor Author

gleonid commented May 20, 2019

I have signed "You have signed the CLA for pingcap/parser" on May 20th 2019 13:03 EST, yet it still shows as not signed in checklist above

@kennytm

This comment has been minimized.

@tiancaiamao
Copy link
Collaborator

LGTM
PTAL @winkyao

@gleonid
Copy link
Contributor Author

gleonid commented Jul 9, 2019

@winkyao
Please decide faith of this PR

I can resolve conflict on parser.go (generated file), but most likely it will become a conflict again as soon as any change to grammar is made (another PR merged).

I have parser.go as a separate commit, specifically for purpose to simplify rebasing.
I am happy to do at any point, or you can do it as easily.

Please let me know how you'd like to proceed

winkyao
winkyao previously approved these changes Jul 18, 2019
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Jul 18, 2019

@gleonid Could you resolve the conflicts?

@winkyao
Copy link
Contributor

winkyao commented Jul 18, 2019

These changes are approved, thanks for your contribution

@gleonid
Copy link
Contributor Author

gleonid commented Jul 18, 2019

@gleonid Could you resolve the conflicts?

@winkyao
conflict has been resolved
unfortunately approval is required again

winkyao
winkyao previously approved these changes Jul 19, 2019
@winkyao
Copy link
Contributor

winkyao commented Jul 19, 2019

The CI is failed with error:

----------------------------------------------------------------------
FAIL: write_test.go:1747: testSuite4.TestQualifiedDelete

write_test.go:1768:
    c.Assert(err, NotNil)
... value = nil

@crazycs520 will help to fix ci.

@kennytm
Copy link
Contributor

kennytm commented Jul 19, 2019

@winkyao The integration test error will be fixed by pingcap/tidb#11184.

@gleonid
Copy link
Contributor Author

gleonid commented Jul 20, 2019

conflict is resolved but now approval is required again

@kennytm kennytm merged commit 191583a into pingcap:master Jul 20, 2019
@gleonid gleonid deleted the issues_280_on_update_ddl branch July 22, 2019 15:29
tangenta pushed a commit to tangenta/parser that referenced this pull request Nov 18, 2019
* issues 280: Support ON UPDATE ddl statements

* issue 280: add generated file
tangenta pushed a commit to tangenta/parser that referenced this pull request Nov 20, 2019
* issues 280: Support ON UPDATE ddl statements

* issue 280: add generated file
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* issues 280: Support ON UPDATE ddl statements

* issue 280: add generated file
ghost pushed a commit to actiontech/parser that referenced this pull request Oct 13, 2021
* issues 280: Support ON UPDATE ddl statements

* issue 280: add generated file

(cherry picked from commit 191583a)
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
* issues 280: Support ON UPDATE ddl statements

* issue 280: add generated file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants