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

fix CONSTRAINT syntax #548

Merged
merged 11 commits into from
Sep 6, 2019
Merged

fix CONSTRAINT syntax #548

merged 11 commits into from
Sep 6, 2019

Conversation

ichn-hu
Copy link
Contributor

@ichn-hu ichn-hu commented Sep 4, 2019

What problem does this PR solve?

Fix #413

What is changed and how it works?

Tests proposed in #413 reflect 2 different aspects of the incompatibility between our parser and MySQL's.

  1. MySQL supports constraints in altering columns, therefore in addtition to modify parser.y to support the syntax, I also added a new attribute NewConstraints in parallel with already exsited NewColumns to AlterTableSpec, and also added the corresponding restoration and visit code. The modification is based on observation of how CreateTableStmt is implemented, because the syntax is similar in MySQL's sql_yacc.yy and our parser.y, and it also uses two different array Cols and Constraints to reflect the 2 different kinds of column definition.
  2. The later 2 test cases in Fix CONSTRAINT syntax #413 is a different incompatibility case, it can be resolved easily as done in sql_yacc.yy

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #548 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
- Coverage   71.77%   71.76%   -0.02%     
==========================================
  Files          32       32              
  Lines        7802     7813      +11     
==========================================
+ Hits         5600     5607       +7     
- Misses       1674     1676       +2     
- Partials      528      530       +2
Impacted Files Coverage Δ
parser.go 70.58% <ø> (ø) ⬆️
ast/ddl.go 80.45% <63.63%> (-0.12%) ⬇️

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 35a39aa...2eee184. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #548 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
- Coverage   71.77%   71.76%   -0.02%     
==========================================
  Files          32       32              
  Lines        7802     7813      +11     
==========================================
+ Hits         5600     5607       +7     
- Misses       1674     1676       +2     
- Partials      528      530       +2
Impacted Files Coverage Δ
parser.go 70.58% <ø> (ø) ⬆️
ast/ddl.go 80.45% <63.63%> (-0.12%) ⬇️

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 35a39aa...23a22c3. Read the comment docs.

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

Rest LGTM

parser.y Outdated Show resolved Hide resolved
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 5, 2019

@tangenta Hi, I've tried hard to increase the coverage, but it still drops, will this block the merge of this PR?

BTW, I'll be very appreciated if you could let me know how to increase the coverage. I figued out that it is because I added code in the visit of AlterTableSpec, however it won't be covered by tests in parser_test.go, since no visitor will be dispatched for parsing.

@zier-one
Copy link
Contributor

zier-one commented Sep 5, 2019

@tangenta Hi, I've tried hard to increase the coverage, but it still drops, will this block the merge of this PR?

BTW, I'll be very appreciated if you could let me know how to increase the coverage. I figued out that it is because I added code in the visit of AlterTableSpec, however it won't be covered by tests in parser_test.go, since no visitor will be dispatched for parsing.

This won't block the merge.

To test Accept function, you can reference https://github.com/pingcap/parser/blob/master/ast/ddl_test.go#L26

@zier-one zier-one changed the title fix CONSTRAINT syntax, close #413 fix CONSTRAINT syntax Sep 5, 2019
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 5, 2019

@tangenta @leoppro I've added two more test cases in https://github.com/pingcap/parser/blob/master/ast/ddl_test.go#L26, not sure if it will have effect on improving coverage.

But if it won't block the merge, please help review the code and merge it, I guess I won't continue to work on improving coverage.

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@tangenta tangenta added the status/LGT1 LGT1 label Sep 6, 2019
Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@zier-one zier-one merged commit e8bb738 into pingcap:master Sep 6, 2019
@zier-one zier-one added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Sep 6, 2019
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix CONSTRAINT syntax
3 participants