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

Support MySQL ROWS syntax for VALUES #737

Merged
merged 3 commits into from Dec 1, 2022
Merged

Support MySQL ROWS syntax for VALUES #737

merged 3 commits into from Dec 1, 2022

Conversation

aljazerzen
Copy link
Contributor

MySQL does not support following query:

VALUES (1)

... but supports ...

VALUES ROW(1)

https://dev.mysql.com/doc/refman/8.0/en/values.html

Note that is does support:

INSERT INTO my_table VALUES (1)

Quality of my code reflects banality of this detail.

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @aljazerzen

The code and tests look good except for the within_insert special case

src/ast/query.rs Show resolved Hide resolved
src/parser.rs Outdated
if parser.parse_keyword(Keyword::ROW) {
explicit_row = true;
} else {
if !within_insert && parser.dialect.values_require_row_in_select() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How important is it to reject the ROWS keyword during inserts? I see that mySQL is not consistent in that ROW is not allowed in INSERT statements https://dev.mysql.com/doc/refman/8.0/en/insert.html

However, I also don't see any (negative) tests for this case in this PR, though maybe I have missed them

My preference is to remove the special case handling for in select and make the parser permissive and support an optional ROW whenever a VALUES list is specified.

Downstream crates that care can detect explicit_row and error if they don't want to allow that syntax.

We tried to make the philosophy of "parse all the things" clearer recently in the introduction here https://github.com/sqlparser-rs/sqlparser-rs#extensible-sql-lexer-and-parser-for-rust

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW I found

Quality of my code reflects banality of this detail.

😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense - there is no need to inforce explict row. And it simplifies the code a lot!

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@coveralls
Copy link

coveralls commented Nov 30, 2022

Pull Request Test Coverage Report for Build 3590726471

  • 39 of 39 (100.0%) changed or added relevant lines in 5 files are covered.
  • 907 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.01%) to 86.36%

Files with Coverage Reduction New Missed Lines %
src/tokenizer.rs 1 90.28%
tests/sqlparser_postgres.rs 24 97.32%
src/ast/query.rs 27 85.32%
tests/sqlparser_common.rs 37 97.25%
src/ast/mod.rs 295 77.39%
src/parser.rs 523 83.56%
Totals Coverage Status
Change from base Build 3577821724: -0.01%
Covered Lines: 12194
Relevant Lines: 14120

💛 - Coveralls

@aljazerzen aljazerzen requested a review from alamb December 1, 2022 08:15
@alamb alamb changed the title Adapt VALUES to MySQL dialect Support MySQL ROWS syntax for VALUES Dec 1, 2022
Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good -- thank you @aljazerzen

@alamb alamb merged commit 8e1c90c into sqlparser-rs:main Dec 1, 2022
@aljazerzen aljazerzen deleted the values branch December 1, 2022 15:33
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.

None yet

3 participants