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.

Sign up for GitHub

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

Jump to bottom

[SPARK-27161][SQL][FOLLOWUP] Drops non-keywords from docs/sql-keywords.md #24125

Closed
wants to merge 4 commits into from
Closed

[SPARK-27161][SQL][FOLLOWUP] Drops non-keywords from docs/sql-keywords.md #24125

wants to merge 4 commits into from

Conversation

maropu
Copy link
Member

@maropu maropu commented Mar 18, 2019

What changes were proposed in this pull request?

This pr is a follow-up of #24093 and includes fixes below;

  • Lists up all the keywords of Spark only (that is, drops non-keywords there); I listed up all the keywords of ANSI SQL-2011 in the previous commit (SPARK-26215).
  • Sorts the keywords in SqlBase.g4 in a alphabetical order

How was this patch tested?

Pass Jenkins.

@maropu
Copy link
Member Author

maropu commented Mar 18, 2019 •
edited

@cloud-fan SETMINUS is not a token there (MINUS instead), so I think this keyword does not show up in the list: https://github.com/apache/spark/pull/24093/files#r265611921

cloud-fan
cloud-fan reviewed Mar 18, 2019
View reviewed changes
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated
@@ -1233,257 +1234,260 @@ nonReserved
| YEARS
;

// NOTE: If you add a new token in the list below, you should update
// a list of keywords in `docs/sql-keywords.md`, too.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a list -> the list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add

If you want to add a non-reserved keyword, please update `ansiNonReserved` and `nonReserved` as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

cloud-fan
cloud-fan reviewed Mar 18, 2019
View reviewed changes
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
SESSION_USER: 'SESSION_USER';
SET: 'SET';
SETMINUS: 'MINUS';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we rename it to MINUS? It's weird to have a keyword that doesn't match its name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll try.

cloud-fan
cloud-fan reviewed Mar 18, 2019
View reviewed changes
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated
@@ -1233,257 +1234,260 @@ nonReserved
| YEARS
;

// NOTE: If you add a new token in the list below, you should update
// a list of keywords in `docs/sql-keywords.md`, too.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add also update the TableIdentifierParserSuite

@cloud-fan
Copy link
Contributor

thanks for the followup!

@SparkQA
Copy link

SparkQA commented Mar 18, 2019

Test build #103610 has finished for PR 24125 at commit c6d57be.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member Author

maropu commented Mar 19, 2019

It seems we cannot use MINUS for that set minus cuz its already used for -;

MINUS: '-';

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103640 has finished for PR 24125 at commit 4f3eaa1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member Author

maropu commented Mar 19, 2019

Missed and I'll update soon.

@maropu
Copy link
Member Author

maropu commented Mar 19, 2019

PERCENT has the same workaround;

PERCENTLIT: 'PERCENT';

@cloud-fan
Copy link
Contributor

thanks for the investigation! LGTM

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103645 has finished for PR 24125 at commit d1b94c9.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103657 has finished for PR 24125 at commit 2627602.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

retest this please

1 similar comment
@maropu
Copy link
Member Author

maropu commented Mar 19, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103663 has finished for PR 24125 at commit 2627602.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 901c740 Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@maropu @cloud-fan @SparkQA