-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-31950][SQL][TESTS] Extract SQL keywords from the generated parser class #28779
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
Conversation
| "except", | ||
| "false", | ||
| "fetch", | ||
| "filter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we forgot to add this word in this list by this change.
| "with", | ||
| "year") | ||
| // All the SQL keywords defined in `SqlBase.g4` | ||
| val allCandidateKeywords = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I've checked that (the existing allCandidateKeywords -- this new allCandidateKeywords) is empty.
|
How about this update? @cloud-fan @viirya @dilipbiswal |
|
Test build #123740 has finished for PR 28779 at commit
|
|
I'm thinking about a text-based solution: we add a special line before the keyword section, and another special line after the key work section, then we just read the sqlbase.g4, find the keword section. We can use it to get ansi reserverd/non-reserved keywords as well. |
|
Ah, I see. I'll update based on the suggestion tomorrow. |
|
Text-based solution sounds good. |
### What changes were proposed in this pull request? This PR intends to extract SQL reserved/non-reserved keywords from the ANTLR grammar file (`SqlBase.g4`) directly. This approach is based on the cloud-fan suggestion: apache#28779 (comment) ### Why are the changes needed? It is hard to maintain a full set of the keywords in `TableIdentifierParserSuite`, so it would be nice if we could extract them from the `SqlBase.g4` file directly. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes apache#28802 from maropu/SPARK-31950-2. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>
### What changes were proposed in this pull request? This PR intends to extract SQL reserved/non-reserved keywords from the ANTLR grammar file (`SqlBase.g4`) directly. This approach is based on the cloud-fan suggestion: #28779 (comment) ### Why are the changes needed? It is hard to maintain a full set of the keywords in `TableIdentifierParserSuite`, so it would be nice if we could extract them from the `SqlBase.g4` file directly. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes #28802 from maropu/SPARK-31950-2. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>
### What changes were proposed in this pull request? This PR intends to extract SQL reserved/non-reserved keywords from the ANTLR grammar file (`SqlBase.g4`) directly. This approach is based on the cloud-fan suggestion: apache#28779 (comment) ### Why are the changes needed? It is hard to maintain a full set of the keywords in `TableIdentifierParserSuite`, so it would be nice if we could extract them from the `SqlBase.g4` file directly. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes apache#28802 from maropu/SPARK-31950-2. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>
What changes were proposed in this pull request?
This PR intends to extract SQL keywords (
allCandidateKeywordsinTableIdentifierParserSuite) from the generated parser class.Why are the changes needed?
It is hard to maintain a full set of SQL keywords in
TableIdentifierParserSuite, so it would be nice if we could updateallCandidateKeywordsautomatically.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.