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 issue 2106: Add parsing functionality for MySQL ADD PARTITION and DROP PARTITION clauses in ALTER TABLE statements(2) #2108

Merged
merged 11 commits into from
Nov 14, 2024

Conversation

minleejae
Copy link
Contributor

@minleejae minleejae commented Nov 13, 2024

Summary
This PR addresses the issue #2016 with JSQLParser's inability to parse ADD PARTITION clauses in ALTER TABLE statements for MySQL 8.0. Previously, these clauses were not recognized, causing parsing errors for certain ALTER TABLE operations.

Changes

  • Modified the parser to correctly identify and handle the ADD PARTITION clauses, including storage options, allowing for accurate parsing of these statements.
  • Added support for parsing DROP PARTITION clauses.
  • Refactored the TRUNCATE PARTITION parsing functionality.

References
MySQL Documentation: ALTER TABLE Partition Operations

@minleejae
Copy link
Contributor Author

Hello,
I'm encountering a StackOverflowError during the compileJava task in the CI environment, as shown in the error log below. However, I haven't been able to reproduce this issue locally, I'd greatly appreciate your guidance.
Notably, this error did not occur before the fix Codacy commit.

Signed-off-by: Andreas Reichel <[email protected]>
Signed-off-by: Andreas Reichel <[email protected]>
Signed-off-by: Andreas Reichel <[email protected]>
@manticore-projects manticore-projects merged commit 12952d6 into JSQLParser:master Nov 14, 2024
1 of 2 checks passed
@manticore-projects
Copy link
Contributor

Thank you for your contribution.
Unfortunately I have no idea yet what causes this Stack Overflow:

  1. I can't re-produce it locally
  2. It worked fine on GitHub one time, only to fail afterwards many times -- without any changes on the Java code

@minleejae
Copy link
Contributor Author

@manticore-projects
Thank you for checking into this. Could the StackOverflowError in the Gradle CI environment be due to an insufficient stack size? I’m not sure, but it might be the issue.

@manticore-projects
Copy link
Contributor

manticore-projects commented Nov 14, 2024

Right now, my uneducated guess was about a build-caching issue because the builds succeed first time and then fail sub-sequentially. We will figure this out eventually.

For example, it just built perfectly fine after merge?!

@manticore-projects
Copy link
Contributor

@minleejae: If you ever come to Thailand for a course of golf, please do let me know!

@minleejae
Copy link
Contributor Author

@manticore-projects
Thank you for the warm welcome!

Regarding the Gradle CI issue, it's possible that the errors we’re seeing might be related to build-caching inconsistencies, which makes it an interesting topic to look into further.

Additionally, I’m not very familiar with JavaCC syntax, I’m curious—does using LOOKAHEAD in JavaCC have any notable impact on performance compared to not using it?

@manticore-projects
Copy link
Contributor

Additionally, I’m not very familiar with JavaCC syntax, I’m curious—does using LOOKAHEAD in JavaCC have any notable impact on performance compared to not using it?

Yes, it does come with a performance penalty although you need to distinguish:

  1. a numeric LOOKAHEAD like LOOKAHEAD(2) is harmless because it will check the next 2 tokens only. In fact I have seen grammars that apply a default LOOKAHEAD(3) to just everything (JSQLParser's default is LOOKAHEAD(1))

  2. a semantic LOOKAHEAD is really expensive and must be avoided at all cost -- although we use it especially for FUNCTION. Unfortunately SQL is a terrible dialect by design with no clear blocks and to make things worse, JSQLParser has grown over 20+ years and also tries to be dialect agnostic. So we have no choice but to engage some functional lookahead -- even when you can actually break the parser with it. For this reason we have COMPLEX vs. SIMPLE parsing modes. All not very beautiful from a technical point of view, but in practice it just works most of the time.

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