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

beliefer · 2020-02-07T02:19:45Z

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

SparkQA · 2020-02-07T06:51:48Z

Test build #118007 has finished for PR 27481 at commit 2f3a54c.

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

maropu · 2020-02-07T13:28:26Z

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

-*/
+-- /* This block comment surrounds a query which itself has a block comment...
+-- SELECT /* embedded single line */ 'embedded' AS x2;
+-- */



Spark SQL can't support nested bracketed comments and I will open another PR to support it.

We need to update this file in this PR? If you will work on that, I think its ok to update this file in the next one.

Thanks for your review. I have a question the nested bracketed comments will throw parsed exception not look good. Should I display the parsed exception into output?

Yea, I think better error messages look good if we can fix it easily.

It won't be easy for the time being. So I want comment out temporarily.

@maropu I think we can fix the test cases in 3.0. Regarding #27495, it is an enhancement, we can merge it to master only.

We need this change to comment out these tests in branch-3.0? If branch-3.0 doesn't support these nested comments, its better to fix them so that the test could throw an exception for nested comments here instead of just commenting out them?

OK. I will not comment out nested comments and throw exception into golden files.

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

SparkQA · 2020-02-11T20:43:47Z

Test build #118251 has finished for PR 27481 at commit c743bf3.

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

SparkQA · 2020-02-11T20:45:09Z

Test build #118252 has finished for PR 27481 at commit 7f21b1b.

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

SparkQA · 2020-02-11T20:51:43Z

Test build #118250 has finished for PR 27481 at commit 4619ac7.

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

maropu · 2020-02-11T23:38:08Z

sql/core/src/test/resources/sql-tests/results/postgreSQL/comments.sql.out

-- !query
-*/
+ * select 'multi-line';
+ */
 SELECT 'after multi-line' AS fifth


Oh, the output is pretty nice.

SparkQA · 2020-02-12T01:26:08Z

Test build #118268 has finished for PR 27481 at commit 5a70f00.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-12T06:33:48Z

Test build #118269 has finished for PR 27481 at commit a512664.

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

cloud-fan · 2020-02-12T11:16:44Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+ *     --QUERY-DELIMITER-START and --QUERY-DELIMITER-END. Lines starting with
+ *     --QUERY-DELIMITER-START and --QUERY-DELIMITER-END represent the beginning and end of a query,
+ *     respectively. Code that is not surrounded by lines that begin with --QUERY-DELIMITER-START
+ *     and --QUERY-DELIMITER-END is still separated by semicolons.


This is better than my original idea. We only need to use this special delimiter for queries that need it. Good job!

cloud-fan · 2020-02-12T11:20:21Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      val otherCodes = new ArrayBuffer[String]
+      var tempStr = ""
+      var start = false
+      for (c <- code) {


code -> importedCode ++ code? The imported code may also have --QUERY-DELIMITER-START

OK. Thanks for your remind.

cloud-fan · 2020-02-12T11:25:55Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+          if (tempStr.endsWith(";")) {
+            tempStr = tempStr.substring(0, tempStr.length - 1)
+          }
+          querys += s"\n$tempStr"


this can be querys += s"\n${tempStr.stripSuffix(";")}"

OK. Good idea.

cloud-fan · 2020-02-12T11:26:59Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+          otherCodes += c
+        }
+      }
+      querys.toSeq


After the lookp ends, it's possible that otherCodes is not empty. We should "flush" it.

OK. I forgot it.

cloud-fan · 2020-02-12T15:53:51Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      for (c <- allCode) {
+        if (c.trim.startsWith("--QUERY-DELIMITER-START")) {
+          start = true
+          querys ++= otherCodes.toSeq.mkString("\n").split("(?<=[^\\\\]);")


This (toSeq.mkString("\n").split("(?<=[^\\\\]);")) appears 3 times, maybe create a function for it?

cloud-fan · 2020-02-12T15:54:01Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+          start = false
+//          if (tempStr.endsWith(";")) {
+//            tempStr = tempStr.substring(0, tempStr.length - 1)
+//          }


let's remove it.

Oh, I forgot it.

SparkQA · 2020-02-12T17:05:25Z

Test build #118298 has finished for PR 27481 at commit 38005e8.

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

maropu · 2020-02-12T23:53:54Z

Looks fine to me except for the existing @cloud-fan comments.

SparkQA · 2020-02-13T05:47:12Z

Test build #118330 has finished for PR 27481 at commit fa8397e.

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

SparkQA · 2020-02-13T08:05:02Z

Test build #118340 has finished for PR 27481 at commit 900cc73.

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

beliefer · 2020-02-13T08:06:06Z

retest this please

SparkQA · 2020-02-13T12:33:10Z

Test build #118342 has finished for PR 27481 at commit 900cc73.

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

cloud-fan · 2020-02-13T14:07:18Z

thanks, merging to master/3.0!

### 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 #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 #27481 from beliefer/ansi-brancket-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 04604b9) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

beliefer · 2020-02-13T15:01:36Z

@cloud-fan @maropu @gatorsmile @dongjoon-hyun Thanks for everyone's work.

### What changes were proposed in this pull request? This PR related to #27481. If test case A uses `--IMPORT` to import test case B contains bracketed comments, the output can't display bracketed comments in golden files well. The content of `nested-comments.sql` show below: ``` -- This test case just used to test imported bracketed comments. -- the first case of bracketed comment --QUERY-DELIMITER-START /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first; --QUERY-DELIMITER-END ``` The test case `comments.sql` imports `nested-comments.sql` below: `--IMPORT nested-comments.sql` Before this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException mismatched input '/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', ' ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == /* This is the first example of bracketed comment. ^^^ SELECT 'ommented out content' AS first -- !query */ SELECT 'selected content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException extraneous input '*/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', 'ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == */ ^^^ SELECT 'selected content' AS first ``` After this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first -- !query schema struct<first:string> -- !query output selected content ``` ### Why are the changes needed? Golden files can't display the bracketed comments in imported test cases. ### Does this PR introduce any user-facing change? 'No'. ### How was this patch tested? New UT. Closes #28018 from beliefer/fix-bug-tests-imported-bracketed-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

### What changes were proposed in this pull request? This PR related to #27481. If test case A uses `--IMPORT` to import test case B contains bracketed comments, the output can't display bracketed comments in golden files well. The content of `nested-comments.sql` show below: ``` -- This test case just used to test imported bracketed comments. -- the first case of bracketed comment --QUERY-DELIMITER-START /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first; --QUERY-DELIMITER-END ``` The test case `comments.sql` imports `nested-comments.sql` below: `--IMPORT nested-comments.sql` Before this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException mismatched input '/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', ' ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == /* This is the first example of bracketed comment. ^^^ SELECT 'ommented out content' AS first -- !query */ SELECT 'selected content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException extraneous input '*/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', 'ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == */ ^^^ SELECT 'selected content' AS first ``` After this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first -- !query schema struct<first:string> -- !query output selected content ``` ### Why are the changes needed? Golden files can't display the bracketed comments in imported test cases. ### Does this PR introduce any user-facing change? 'No'. ### How was this patch tested? New UT. Closes #28018 from beliefer/fix-bug-tests-imported-bracketed-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org> (cherry picked from commit 9e0fee9) Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

### 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? This PR related to apache#27481. If test case A uses `--IMPORT` to import test case B contains bracketed comments, the output can't display bracketed comments in golden files well. The content of `nested-comments.sql` show below: ``` -- This test case just used to test imported bracketed comments. -- the first case of bracketed comment --QUERY-DELIMITER-START /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first; --QUERY-DELIMITER-END ``` The test case `comments.sql` imports `nested-comments.sql` below: `--IMPORT nested-comments.sql` Before this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException mismatched input '/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', ' ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == /* This is the first example of bracketed comment. ^^^ SELECT 'ommented out content' AS first -- !query */ SELECT 'selected content' AS first -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException extraneous input '*/' expecting {'(', 'ADD', 'ALTER', 'ANALYZE', 'CACHE', 'CLEAR', 'COMMENT', 'COMMIT', 'CREATE', 'DELETE', 'DESC', 'DESCRIBE', 'DFS', 'DROP', 'EXPLAIN', 'EXPORT', 'FROM', 'GRANT', 'IMPORT', 'INSERT', 'LIST', 'LOAD', 'LOCK', 'MAP', 'MERGE', 'MSCK', 'REDUCE', 'REFRESH', 'REPLACE', 'RESET', 'REVOKE', 'ROLLBACK', 'SELECT', 'SET', 'SHOW', 'START', 'TABLE', 'TRUNCATE', 'UNCACHE', 'UNLOCK', 'UPDATE', 'USE', 'VALUES', 'WITH'}(line 1, pos 0) == SQL == */ ^^^ SELECT 'selected content' AS first ``` After this PR, the output will be: ``` -- !query /* This is the first example of bracketed comment. SELECT 'ommented out content' AS first; */ SELECT 'selected content' AS first -- !query schema struct<first:string> -- !query output selected content ``` ### Why are the changes needed? Golden files can't display the bracketed comments in imported test cases. ### Does this PR introduce any user-facing change? 'No'. ### How was this patch tested? New UT. Closes apache#28018 from beliefer/fix-bug-tests-imported-bracketed-comments. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

Improve bracketed comments tests.

2f3a54c

maropu changed the title ~~[SPARK-28880][SQL] Improve bracketed comments tests.~~ [SPARK-28880][SQL][TESTS] Improve bracketed comments tests. Feb 7, 2020

maropu reviewed Feb 7, 2020

View reviewed changes

beliefer mentioned this pull request Feb 8, 2020

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

Closed

beliefer changed the title ~~[SPARK-28880][SQL][TESTS] Improve bracketed comments tests.~~ [SPARK-30758][SQL][TESTS] Improve bracketed comments tests. Feb 8, 2020

cloud-fan reviewed Feb 10, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala Show resolved Hide resolved

dongjoon-hyun added the SQL label Feb 10, 2020

beliefer added 3 commits February 11, 2020 23:15

Adjust code

4619ac7

Revert PlanParserSuite

c743bf3

Add comment.

7f21b1b

maropu reviewed Feb 11, 2020

View reviewed changes

beliefer added 3 commits February 12, 2020 09:18

Update how to use --QUERY-DELIMITER

71e4c43

Update how to use --QUERY-DELIMITER

5a70f00

Update how to use --QUERY-DELIMITER

3f8497f

Update how to use --QUERY-DELIMITER

a512664

cloud-fan reviewed Feb 12, 2020

View reviewed changes

Optimize code

38005e8

cloud-fan reviewed Feb 12, 2020

View reviewed changes

Optimize code

fa8397e

maropu approved these changes Feb 13, 2020

View reviewed changes

Update comment

900cc73

cloud-fan closed this in 04604b9 Feb 13, 2020

beliefer mentioned this pull request Mar 25, 2020

[SPARK-31262][SQL][TESTS] Fix bug tests imported bracketed comments #28018

Closed

beliefer deleted the ansi-brancket-comments branch April 23, 2024 06:24

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

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

Conversation

beliefer commented Feb 7, 2020 • edited Loading

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?

SparkQA commented Feb 7, 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

maropu Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 11, 2020

SparkQA commented Feb 11, 2020

SparkQA commented Feb 11, 2020

Choose a reason for hiding this comment

SparkQA commented Feb 12, 2020

SparkQA commented Feb 12, 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

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 12, 2020

maropu commented Feb 12, 2020

SparkQA commented Feb 13, 2020

SparkQA commented Feb 13, 2020

beliefer commented Feb 13, 2020

SparkQA commented Feb 13, 2020

cloud-fan commented Feb 13, 2020

beliefer commented Feb 13, 2020

beliefer commented Feb 7, 2020 •

edited

Loading

maropu Feb 9, 2020 •

edited

Loading