Skip to content

Add support for OFFSET without the ROWS keyword #158

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 1 commit into from
Apr 20, 2020
Merged

Add support for OFFSET without the ROWS keyword #158

merged 1 commit into from
Apr 20, 2020

Conversation

maddyblue
Copy link
Contributor

MySQL doesn't support the ROWS part of OFFSET. Teach the parser to
remember which variant it saw, including just ROW.

@maddyblue
Copy link
Contributor Author

Test failed due to a rustfmt nightly issue. Bumping to re-run.

@maddyblue
Copy link
Contributor Author

Ok well this keeps failing with error: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is unavailable for download for channel nightly which I don't think is related to this PR.

@maddyblue
Copy link
Contributor Author

Ahh https://github.com/andygrove/sqlparser-rs/pull/159 seems to maybe fix it.

@nickolay
Copy link
Contributor

Yep, the rustfmt issue should be fixed in #159. I didn't manage to fix coverage yesterday, before I got distracted.

Sorry you didn't get a reply. I took a quick glance at the patch yesterday - and it looked great (thanks!), but I wanted to fix CI first before taking a closer look and merging.

Note to self: this quirk is documented in https://dev.mysql.com/doc/refman/8.0/en/select.html

For compatibility with PostgreSQL, MySQL also supports the LIMIT row_count OFFSET offset syntax.

(note the lack of ROWS after offset)

MySQL doesn't support the ROWS part of OFFSET. Teach the parser to
remember which variant it saw, including just ROW.
@maddyblue
Copy link
Contributor Author

Rebased on master.

@nickolay nickolay changed the title Add support for OFFSET with the ROWS keyword Add support for OFFSET without the ROWS keyword Apr 20, 2020
@nickolay nickolay merged commit af852e7 into apache:master Apr 20, 2020
@nickolay
Copy link
Contributor

Excellent (and thanks for rebasing promptly)!

Do I understand it correctly that you needed MySQL-specific syntax to round-trip properly for your use-case?

@maddyblue maddyblue deleted the offset-rows branch April 20, 2020 02:39
@maddyblue
Copy link
Contributor Author

Luckily I am currently only producing SQL, so things are fine as is. I do have an upcoming project that would require me to parse SQL, and this is one of the first obvious things that would force me to do silly stuff like use regexes to remove or insert the keywords. I took a look at your custom parser doc because I was curious how this could be possible with the current code.

nickolay added a commit that referenced this pull request Apr 20, 2020
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.

2 participants