[SPARK-31950][SQL][TESTS] Extract SQL keywords from the SqlBase.g4 file #28802

maropu · 2020-06-11T15:59:01Z

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.

cloud-fan · 2020-06-11T16:58:51Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

 // The non-reserved keywords are listed in `nonReserved`.
 // These 2 together contain all the keywords.
 strictNonReserved
+//--ANSI-STRICT-NON-RESERVED-START


this is not ansi, we should just call it STRICT-NON-RESERVED

oh, I missed that. I'll update soon.

cloud-fan · 2020-06-11T17:04:16Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

+  val nonReservedKeywordsInAnsiMode = parseSyntax(
+    "//--ANSI-NON-RESERVED-START", "//--ANSI-NON-RESERVED-END")
+
+  val reservedKeywordsInAnsiMode = {


ansi reserved keywords are "full keywords -- non-reserved keywords".

"non-reserved keywords" means non-reserved keywords in ansi + strict non-reserved keywords?

Does previous allCandidateKeywords include strict non-reserved keywords?

okay, fixed.

SparkQA · 2020-06-11T21:48:10Z

Test build #123860 has finished for PR 28802 at commit fdd1bcb.

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

dilipbiswal · 2020-06-11T23:34:17Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

+  private val sqlSyntaxDefs = {
+    val sqlBasePath = {
+      val sparkHome = {
+        assert(sys.props.contains("spark.test.home") ||


@maropu Minor Nit. I was trying to run this and i didn't have the environment variable set. The error message ends up printing the full env info making it difficult to find what the error is ? Should we use something like:
if (condition) {
fail(...)
}
or may be there is a better way :-).

Thanks for the check, @dilipbiswal . SQLQueryTestSuite has the same logic, so I personally think its better to fix them at the same time. https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L132
Could you make a PR after this merged?

@maropu Sure.. will do.

viirya · 2020-06-11T23:51:51Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

+      if (line.trim.startsWith(startTag)) {
+        start = true
+      } else if (line.trim.startsWith(endTag)) {
+        start = false


nit: Just return once reaching endTag?

viirya · 2020-06-11T23:54:01Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

+    "//--ANSI-NON-RESERVED-START", "//--ANSI-NON-RESERVED-END")

-  val nonReservedKeywordsInAnsiMode = allCandidateKeywords -- reservedKeywordsInAnsiMode
+  val reservedKeywordsInAnsiMode = allCandidateKeywords -- nonReservedKeywordsInAnsiMode


If we remove the tags, will the related tests fail? I have this question because these tests seems do foreach on these lists. If we remove the tags and get an empty list, seems the tests won't fail too?

Ah, nice suggestion. In that case, the tests should fail. I'll check and update.

Could you check the latest commit? Is that okay to you?

Yeah, thanks. Looks good.

cloud-fan · 2020-06-12T04:49:56Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

+  }

-  val nonReservedKeywordsInAnsiMode = allCandidateKeywords -- reservedKeywordsInAnsiMode
+  private def parseSyntax(startTag: String, endTag: String): Set[String] = {


nit: it's only used once, can we inline it?

Ah, right. okay.

SparkQA · 2020-06-12T04:51:00Z

Test build #123871 has finished for PR 28802 at commit d748882.

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

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala

SparkQA · 2020-06-12T07:05:01Z

Test build #123874 has finished for PR 28802 at commit 349b919.

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

SparkQA · 2020-06-12T07:05:02Z

Test build #123891 has finished for PR 28802 at commit 09a2ed4.

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

SparkQA · 2020-06-12T07:05:02Z

Test build #123892 has finished for PR 28802 at commit 80b1c3a.

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

SparkQA · 2020-06-12T07:05:02Z

Test build #123876 has finished for PR 28802 at commit 3c1ce72.

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

SparkQA · 2020-06-12T15:14:00Z

Test build #123905 has finished for PR 28802 at commit bd2c6bc.

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

maropu · 2020-06-12T22:13:38Z

Thanks! Merged to master.

### 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]>

Fix

fdd1bcb

maropu mentioned this pull request Jun 11, 2020

[SPARK-31950][SQL][TESTS] Extract SQL keywords from the generated parser class #28779

Closed

cloud-fan reviewed Jun 11, 2020

View reviewed changes

dilipbiswal reviewed Jun 11, 2020

View reviewed changes

review

d748882

probot-autolabeler bot added the SQL label Jun 11, 2020

viirya reviewed Jun 11, 2020

View reviewed changes

maropu added 2 commits June 12, 2020 10:10

review

349b919

minor fix

3c1ce72

cloud-fan reviewed Jun 12, 2020

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/TableIdentifierParserSuite.scala Show resolved Hide resolved

maropu added 2 commits June 12, 2020 14:00

review

09a2ed4

Fix

80b1c3a

fix

bd2c6bc

cloud-fan approved these changes Jun 12, 2020

View reviewed changes

viirya approved these changes Jun 12, 2020

View reviewed changes

maropu closed this in 78d08a8 Jun 12, 2020

maropu mentioned this pull request Jun 15, 2020

[SPARK-26905][SQL] Follow the SQL:2016 reserved keywords #28807

Closed

[SPARK-31950][SQL][TESTS] Extract SQL keywords from the SqlBase.g4 file #28802

[SPARK-31950][SQL][TESTS] Extract SQL keywords from the SqlBase.g4 file #28802

Uh oh!

Conversation

maropu commented Jun 11, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 11, 2020

Uh oh!

dilipbiswal Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

maropu commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya Jun 11, 2020 •

edited

Loading

dilipbiswal Jun 11, 2020 •

edited

Loading