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

Allow specific keywords to be parsed as identifiers (fix issue with RENAME COLUMN) #2203

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

minleejae
Copy link
Contributor

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!

@manticore-projects
Copy link
Contributor

We need to rework the whole keyword management eventually. But in the meantime this can be a remedy.

@manticore-projects manticore-projects merged commit a068dd4 into JSQLParser:master Mar 24, 2025
3 of 4 checks passed
@minleejae
Copy link
Contributor Author

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.

@manticore-projects
Copy link
Contributor

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:

  1. switch to JavaCC 8 (or even CongoCC)
  2. then rework the DDL statements (they are a big mess)
  3. then rewrite the KEYWORD handling

@minleejae
Copy link
Contributor Author

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?

@manticore-projects
Copy link
Contributor

manticore-projects commented Mar 24, 2025

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.

@minleejae
Copy link
Contributor Author

@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.

@manticore-projects
Copy link
Contributor

manticore-projects commented Mar 25, 2025

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.

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