GeezerVison is showing github.com — Allow specific keywords to be parsed as identifiers (fix issue with RENAME COLUMN) by minleejae · Pull Request #2203 · JSQLParser/JSqlParser
Conversation
Summary
This PR introduces a new parser rule KeywordOrIdentifier() to handle cases where certain keywords are used as identifiers. This addresses issues that arose after specific tokens like NAME were introduced as keywords.
Problem
While parsing a statement like:
ALTER TABLE test_table RENAME COLUMN name TO full_name;
the parser failed because name was recognized strictly as a <K_NAME> keyword after this commit, instead of being allowed as an identifier.
Solution
- Introduced
KeywordOrIdentifier() rule that accepts:
<S_IDENTIFIER>
<S_QUOTED_IDENTIFIER>
- Selected keywords like
<K_NAME>, <K_NEXT>, <K_VALUE>, <K_PUBLIC>, <K_STRING>
- Additional tokens can be added as needed in the future.
- Replaced relevant parts of the grammar to use
KeywordOrIdentifier().
I’m open to any feedback or suggestions on this approach!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
We need to rework the whole keyword management eventually. But in the meantime this can be a remedy.
|
minleejae reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Hi, @manticore-projects
I also agree that the current keyword handling could use a more systematic approach.
Out of curiosity, could you share your thoughts on what the ultimate direction for keyword management should look like? I'd like to align any future refactoring efforts accordingly.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The current implementation has grown more or less wild over 20 years and based on demand. It also suffers limitations from JavaCC 7.
At the same time it somehow just works good enough for most use-cases and rewriting it will take a lot of effort.
So my current idea was:
- switch to JavaCC 8 (or even CongoCC)
- then rework the DDL statements (they are a big mess)
- then rewrite the KEYWORD handling
|
minleejae reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Hi, @manticore-projects
This sounds like quite a large-scale refactoring effort. I’m curious — do you have any rough timeline or plan in mind for when you'd like to tackle this?
Also, just wondering, have you ever considered adopting something like ANTLR instead, or is the plan to stick with JavaCC/CongoCC going forward?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
ANTLR has been found producing slower code and so is out of scope for me. Although this claim could need a verification again.
Also, when rebasing to ANTLR then we would/should have to rewrite from scratch.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@manticore-projects
It's interesting that ANTLR was found to be slow. I wonder if that has something to do with the structural differences between LL and LR parsers. Of course, replacing ANTLR would come with the high cost of building everything from scratch, but I thought one of its strengths was the ability to support multiple languages.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I am not doubting the strength or superiority of ANTLR in general but only point out why I was more interested in sticking with the JavaCC origin as long as possible. JavaCC/CongoCC also supports multiple languages although I am really interested in Java only (and maybe C).
For my own purpose (real time parsing SQL while typing) I am interested in the fastest (automatically generated) parser possible -- and so far JavaCC holds that position.
|
minleejae reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|