Skip to content

Refactor <column definition> parsing #251

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

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

nickolay
Copy link
Contributor

Since PR #93
parse_column_def parses a set of column options in a loop, e.g. given:

                  _______ column_def _______
CREATE TABLE foo (bar INT NOT NULL DEFAULT 1, )
                          -------- ---------
                          option 1  option 2

it parses column options until it encounters one of the delimiter tokens

First when we only supported CREATE TABLE, the set of delimiters that
stopped the parsing used to be Token::Comma | Token::RParen.

Then we added support for ALTER TABLE ADD COLUMN <column_def>. Turns
out the parser started to bail if the statement ended with a semicolon,
while attempting to parse the semicolon as a column option, as we forgot
to add it to the set of delimiter tokens.

This was recently fixed in #246
by including Token::SemiColon to the list, but it felt wrong to have
to update this list, and to have a common list of delimiters for two
different contexts (CREATE TABLE with parens vs ALTER TABLE ADD COLUMN
without parens).

Also our current approach cannot handle multiple statements NOT
separated by a semicolon, as is common in MS SQL DDL. We don't
explicitly support it in parse_statements, but that's a use-case
like to keep in mind nevertheless.

@nickolay nickolay requested a review from Dandandan July 31, 2020 15:24
@nickolay nickolay force-pushed the pr/parse_column_def branch from 041efad to 8106d25 Compare July 31, 2020 15:26
@coveralls
Copy link

coveralls commented Jul 31, 2020

Pull Request Test Coverage Report for Build 202608611

  • 37 of 39 (94.87%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 91.92%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 35 37 94.59%
Totals Coverage Status
Change from base Build 202579293: 0.001%
Covered Lines: 4539
Relevant Lines: 4938

💛 - Coveralls

@nickolay
Copy link
Contributor Author

nickolay commented Aug 5, 2020

This might conflict with #254, so I'm going to postpone landing this until that PR is merged.

...because in the section related to CREATE TABLE parsing, the callers
are defined above the callees.
Since PR apache#93
`parse_column_def` parses a set of column options in a loop, e.g. given:

```
                  _______ column_def _______
CREATE TABLE foo (bar INT NOT NULL DEFAULT 1, )
                          -------- ---------
                          option 1  option 2
````

it parses column options until it encounters one of the delimiter tokens

First when we only supported `CREATE TABLE`, the set of delimiters that
stopped the parsing used to be `Token::Comma | Token::RParen`.

Then we added support for `ALTER TABLE ADD COLUMN <column_def>`. Turns
out the parser started to bail if the statement ended with a semicolon,
while attempting to parse the semicolon as a column option, as we forgot
to add it to the set of delimiter tokens.

This was recently fixed in apache#246
by including Token::SemiColon to the list, but it felt wrong to have
to update this list, and to have a common list of delimiters for two
different contexts (CREATE TABLE with parens vs ALTER TABLE ADD COLUMN
without parens).

Also our current approach cannot handle multiple statements NOT
separated by a semicolon, as is common in MS SQL DDL. We don't
explicitly support it in `parse_statements`, but that's a use-case
like to keep in mind nevertheless.
@nickolay nickolay force-pushed the pr/parse_column_def branch from adb7b76 to 66505eb Compare August 10, 2020 14:13
@nickolay nickolay merged commit 2c6c295 into apache:main Aug 10, 2020
@nickolay nickolay deleted the pr/parse_column_def branch August 10, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants