[SPARK-28880][SQL] Support ANSI nested bracketed comments #27495

beliefer · 2020-02-08T03:15:55Z

What changes were proposed in this pull request?

Spark SQL support single comments and bracketed comments now. This PR will support nested bracketed comments.

There are some mainstream database support the syntax.
PostgreSQL:
https://www.postgresql.org/docs/11/sql-syntax-lexical.html#SQL-SYNTAX-COMMENTS

Vertica:
https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Expressions/Comments.htm?zoom_highlight=comments

Note: Because Spark SQL not exists UT for single comments and bracketed comments, so I add some UT for them.

Why are the changes needed?

nested bracketed comments is ANSI standard.

Does this PR introduce any user-facing change?

No

How was this patch tested?

New UT

gatorsmile · 2020-02-08T04:47:22Z

cc @maropu @gengliangwang @cloud-fan

beliefer · 2020-02-08T05:13:39Z

@gatorsmile Thank you.

SparkQA · 2020-02-08T07:24:49Z

Test build #118054 has finished for PR 27495 at commit 464cd03.

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

maropu · 2020-02-08T08:47:54Z

I personally think that these supported comment syntax are documented in the Spark SQL Guide (along with the PostgreSQL/Vertica docs above). WDYT? cc: @dilipbiswal

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

maropu · 2020-02-08T09:00:12Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

+
+  test("bracketed comment case one") {
+    val plan = table("a").select(star())
+    assertEqual("/* This is an example of SQL which should not execute:\n" +


Could you follow a format like this?

assertEqual( """ |/* | XXX | */ |SELECT ... """.stripMargin,

OK. Thanks.

maropu · 2020-02-08T10:04:39Z

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

@@ -1794,7 +1794,7 @@ BRACKETED_EMPTY_COMMENT
    ;

 BRACKETED_COMMENT
-    : '/*' ~[+] .*? '*/' -> channel(HIDDEN)
+    : '/*' ~[+] ( ~'/' | ~'*' '/' ~'*' )*? BRACKETED_COMMENT? ( ~'/' | ~'*' '/' ~'*' )*? '*/' -> channel(HIDDEN)


: '/*' ~[+] (BRACKETED_COMMENT|.)*? '*/' -> channel(HIDDEN) ?

I'm sorry. Maybe I lost some information.
I try to use this again and found it works well.

maropu · 2020-02-08T10:23:52Z

Can you remove the jira numbers below?

spark/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/comments.sql

Line 14 in 12e1bba

-- [SPARK-28880] ANSI SQL: Bracketed comments

spark/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/comments.sql

Line 20 in 12e1bba

-- [SPARK-28880] ANSI SQL: Bracketed comments

beliefer · 2020-02-08T10:51:09Z

Can you remove the jira numbers below?

spark/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/comments.sql

Line 14 in 12e1bba

-- [SPARK-28880] ANSI SQL: Bracketed comments

spark/sql/core/src/test/resources/sql-tests/inputs/postgreSQL/comments.sql

Line 20 in 12e1bba

-- [SPARK-28880] ANSI SQL: Bracketed comments

I want remove the jira numbers with #27481. Golden files can't generated correctly.

maropu · 2020-02-08T11:56:42Z

If the golden file is not correct, we should fix this in this pr because SPARK-28880 is a ticket to support nested bracketed comments and this means the fix makes the file correct. To resolve SPARK-28880, I think we need to check if the golden file is correct.

beliefer · 2020-02-08T11:58:52Z

If the golden file is not correct, we should fix this in this pr because SPARK-28880 is a ticket to support nested bracketed comments and this means the fix makes the file correct. To resolve SPARK-28880, I think we need to check if the golden file is correct.

Could I create another ticket and update golden file with the new ticket id?

SparkQA · 2020-02-08T12:18:50Z

Test build #118063 has finished for PR 27495 at commit c782bac.

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

maropu · 2020-02-08T13:27:21Z

Since that's a minor fix, plz include it in this pr.

SparkQA · 2020-02-08T13:37:49Z

Test build #118066 has finished for PR 27495 at commit 0a21b5e.

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

SparkQA · 2020-02-08T15:02:29Z

Test build #118069 has finished for PR 27495 at commit 1b9d1da.

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

SparkQA · 2020-02-08T17:00:42Z

Test build #118070 has finished for PR 27495 at commit 3d03688.

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

SparkQA · 2020-02-08T17:08:19Z

Test build #118071 has finished for PR 27495 at commit 596d905.

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

beliefer · 2020-02-08T23:36:24Z

Since that's a minor fix, plz include it in this pr.

I have no idea that SQLQueryTestSuite treats nested bracketed comments well now.
#27481 treats bracketed comments well now.
I don't want the work for SQLQueryTestSuite blocks support for nested bracketed comments.
And I think these are two things.

maropu · 2020-02-09T02:04:09Z

Ah, I got it now. The reason not to generate the output correctly is an issue in the testing logic itself in SQLQueryTestSuite, right? (I thought that's an issue in how-to-write postgreSQL/comments.sql). If so, they're different issues as you said above.

beliefer · 2020-02-09T02:55:29Z

Ah, I got it now. The reason not to generate the output correctly is an issue in the testing logic itself in SQLQueryTestSuite, right? (I thought that's an issue in how-to-write postgreSQL/comments.sql). If so, they're different issues as you said above.

@maropu Yes, thank you. I will throw exception in golden files until we can fix it.

SparkQA · 2020-02-09T12:36:59Z

Test build #118095 has finished for PR 27495 at commit 7760f91.

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

beliefer · 2020-02-21T09:00:12Z

@cloud-fan This is a very very good idea. I learned it and will make a try.

beliefer · 2020-02-21T09:37:36Z

@cloud-fan
'/*' (BRACKETED_COMMENT|.)*? '*/' {isHint()}? -> channel(HIDDEN)
exists a issue that antlr matches bracketed comments or hint first and then call method isHint, so the matches will throw parse exception first.

beliefer · 2020-02-21T10:36:44Z

@cloud-fan @gengliangwang
I improved the code show below and it works well.

public boolean isHint() {
   int firstChar = _input.LA(1);
   if (firstChar == '+') {
     return true;
   } else {
     return false;
   }
}
'/*' {!isHint()}? (BRACKETED_COMMENT|.)*? '*/' -> channel(HIDDEN)

cloud-fan · 2020-02-21T10:37:01Z

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

+   * Returns true if the first character is '+'.
+   */
+  public boolean isHint() {
+    int firstChar = _input.LA(1);


_input.LA(1) returns next char, shall we name it nextChar?

cloud-fan · 2020-02-21T10:38:26Z

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

@@ -61,6 +61,19 @@ grammar SqlBase;
   * When true, the behavior of keywords follows ANSI SQL standard.
   */
  public boolean SQL_standard_keyword_behavior = false;
+
+  /**
+   * Verify whether current token is a valid hint token (which follows '/*' and is '+').


I'd like to enrich this comment:

This is called when we see '/*' and try to match it as a comment. If the next char is '+', this should be parsed as hint later and we can't match it as a comment.

This is better. Thanks.

cloud-fan · 2020-02-21T10:38:56Z

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

@@ -1797,11 +1810,11 @@ SIMPLE_COMMENT
    ;

 BRACKETED_EMPTY_COMMENT


do we still need it?

(BRACKETED_COMMENT|.)*? should work for empty comment

SparkQA · 2020-02-21T10:59:26Z

Test build #118785 has finished for PR 27495 at commit 660cd34.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-21T11:23:34Z

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

+
+  /**
+   * This method will be called when we see '/*' and try to match it as a bracketed comment.
+   * If the next character is '+', it should be parsed as hint later, otherwise we cannot match


otherwise -> and

cloud-fan · 2020-02-21T11:23:47Z

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

+   * If the next character is '+', it should be parsed as hint later, otherwise we cannot match
+   * it as a bracketed comment.
+   *
+   * Returns true if the first character is '+'.


next character

SparkQA · 2020-02-21T11:51:02Z

Test build #118788 has finished for PR 27495 at commit 48814c3.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-21T11:52:34Z

retest this please

SparkQA · 2020-02-21T12:15:08Z

Test build #118789 has finished for PR 27495 at commit 48814c3.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-21T12:18:37Z

retest this please

SparkQA · 2020-02-21T12:39:50Z

Test build #118790 has finished for PR 27495 at commit 48814c3.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

beliefer · 2020-02-21T15:58:26Z

retest this please

SparkQA · 2020-02-21T16:09:18Z

Test build #118786 has finished for PR 27495 at commit 9c856b7.

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

SparkQA · 2020-02-21T20:48:38Z

Test build #118798 has finished for PR 27495 at commit 48814c3.

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

gengliangwang

LGTM, thanks for the work!

gengliangwang · 2020-02-24T08:28:25Z

Merging to master.

beliefer · 2020-02-24T08:36:35Z

@cloud-fan @maropu @gengliangwang @dongjoon-hyun @gatorsmile Thanks for all your help.

### What changes were proposed in this pull request? Although Spark SQL support bracketed comments, but `SQLQueryTestSuite` can't treat bracketed comments well and lead to generated golden files can't display bracketed comments well. This PR will improve the treatment of bracketed comments and add three test case in `PlanParserSuite`. Spark SQL can't support nested bracketed comments and apache#27495 used to support it. ### Why are the changes needed? Golden files can't display well. ### Does this PR introduce any user-facing change? No ### How was this patch tested? New UT. Closes apache#27481 from beliefer/ansi-brancket-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? Spark SQL support single comments and bracketed comments now. This PR will support nested bracketed comments. There are some mainstream database support the syntax. **PostgreSQL:** https://www.postgresql.org/docs/11/sql-syntax-lexical.html#SQL-SYNTAX-COMMENTS **Vertica:** https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Expressions/Comments.htm?zoom_highlight=comments Note: Because Spark SQL not exists UT for single comments and bracketed comments, so I add some UT for them. ### Why are the changes needed? nested bracketed comments is ANSI standard. ### Does this PR introduce any user-facing change? No ### How was this patch tested? New UT Closes apache#27495 from beliefer/nested-brancket-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com>

Support nested bracketed comments

464cd03

Fix bug

c782bac

maropu reviewed Feb 8, 2020

View reviewed changes

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

maropu reviewed Feb 8, 2020

View reviewed changes

Optimize code

0a21b5e

maropu reviewed Feb 8, 2020

View reviewed changes

Add test cases

1b9d1da

beliefer mentioned this pull request Feb 8, 2020

[SPARK-30758][SQL][TESTS] Improve bracketed comments tests. #27481

Closed

beliefer added 2 commits February 8, 2020 20:56

Update jira comment.

3d03688

Update jira comment.

596d905

release comment

7760f91

Update test cases.

efe98fa

Optimize code

660cd34

cloud-fan reviewed Feb 21, 2020

View reviewed changes

Optimize code

9c856b7

cloud-fan reviewed Feb 21, 2020

View reviewed changes

cloud-fan approved these changes Feb 21, 2020

View reviewed changes

Optimize code

48814c3

gengliangwang approved these changes Feb 24, 2020

View reviewed changes

gengliangwang closed this in 621e37e Feb 24, 2020

beliefer deleted the nested-brancket-comments branch April 23, 2024 06:37

		@@ -1797,11 +1810,11 @@ SIMPLE_COMMENT
		;

		BRACKETED_EMPTY_COMMENT

[SPARK-28880][SQL] Support ANSI nested bracketed comments #27495

[SPARK-28880][SQL] Support ANSI nested bracketed comments #27495

Conversation

beliefer commented Feb 8, 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?

gatorsmile commented Feb 8, 2020

beliefer commented Feb 8, 2020

SparkQA commented Feb 8, 2020

maropu commented Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

beliefer Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

maropu commented Feb 8, 2020

beliefer commented Feb 8, 2020

maropu commented Feb 8, 2020

beliefer commented Feb 8, 2020

SparkQA commented Feb 8, 2020

maropu commented Feb 8, 2020

SparkQA commented Feb 8, 2020

SparkQA commented Feb 8, 2020

SparkQA commented Feb 8, 2020

SparkQA commented Feb 8, 2020

beliefer commented Feb 8, 2020 • edited Loading

maropu commented Feb 9, 2020 • edited Loading

beliefer commented Feb 9, 2020 • edited Loading

SparkQA commented Feb 9, 2020

beliefer commented Feb 21, 2020

beliefer commented Feb 21, 2020

beliefer commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 21, 2020

cloud-fan commented Feb 21, 2020

SparkQA commented Feb 21, 2020

cloud-fan commented Feb 21, 2020

SparkQA commented Feb 21, 2020

beliefer commented Feb 21, 2020

SparkQA commented Feb 21, 2020

SparkQA commented Feb 21, 2020

gengliangwang left a comment

Choose a reason for hiding this comment

gengliangwang commented Feb 24, 2020

beliefer commented Feb 24, 2020

maropu commented Feb 8, 2020 •

edited

Loading

maropu Feb 8, 2020 •

edited

Loading

beliefer Feb 8, 2020 •

edited

Loading

beliefer commented Feb 8, 2020 •

edited

Loading

maropu commented Feb 9, 2020 •

edited

Loading

beliefer commented Feb 9, 2020 •

edited

Loading