[SPARK-31102][SQL] Spark-sql fails to parse when contains comment. #27920

javierivanov · 2020-03-16T02:11:43Z

What changes were proposed in this pull request?

This PR introduces a change to false for the insideComment flag on a newline. Fixing the issue introduced by SPARK-30049.

Why are the changes needed?

Previously on SPARK-30049 a comment containing an unclosed quote produced the following issue:

spark-sql> SELECT 1 -- someone's comment here
         > ;
Error in query: 
extraneous input ';' expecting <EOF>(line 2, pos 0)

== SQL ==
SELECT 1 -- someone's comment here
;
^^^

This was caused because there was no flag for comment sections inside the splitSemiColon method to ignore quotes. SPARK-30049 added that flag and fixed the issue, but introduced the follwoing problem:

spark-sql> select
         >   1,
         >   -- two
         >   2;
Error in query:
mismatched input '<EOF>' expecting {'(', 'ADD', 'AFTER', 'ALL', 'ALTER', ...}(line 3, pos 2)
== SQL ==
select
  1,
--^^^

This issue is generated by a missing turn-off for the insideComment flag with a newline.

Does this PR introduce any user-facing change?

No

How was this patch tested?

For previous tests using line-continuity(\) it was added a line-continuity rule in the SqlBase.g4 file to add the functionality to the SQL context.
A new test for inline comments was added.

maropu · 2020-03-16T02:42:29Z

ok to test

SparkQA · 2020-03-16T03:19:42Z

Test build #119825 has finished for PR 27920 at commit d69d271.

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

maropu · 2020-03-16T04:55:20Z

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala

-      """SELECT concat('test', 'comment') -- someone's comment here \\
-        |   comment continues here with single ' quote \\
-        |   extra ' \\
-        |   ;""".stripMargin -> "testcomment"


Why you did you remove the existing tests instead of adding new tests?

Hey @maropu !
The SQL parser does not recognize line-continuity per se.

scala> sql(s"""SELECT concat('test', 'comment') -- someone's comment here \\\ncomment continues here with single ' quote \\\nextra ' \\""") org.apache.spark.sql.catalyst.parser.ParseException: mismatched input 'continues' expecting {<EOF>, ',', 'CLUSTER', 'DISTRIBUTE', 'EXCEPT', 'FROM', 'GROUP', 'HAVING', 'INTERSECT', 'LATERAL', 'LIMIT', 'ORDER', 'MINUS', 'SORT', 'UNION', 'WHERE', 'WINDOW', '-'}(line 2, pos 8) == SQL == SELECT concat('test', 'comment') -- someone's comment here \ comment continues here with single ' quote \ --------^^^ extra ' \

It works just fine for inline comments included backslash:

scala> sql(s"""SELECT concat('test', 'comment') -- someone's comment here \\\n,2""") show +---------------------+---+ |concat(test, comment)| 2| +---------------------+---+ | testcomment| 2| +---------------------+---+

But does not work outside the inline comment(the backslash):

sql(s"""SELECT concat('test', 'comment') -- someone's comment here \n,2\\\n""") org.apache.spark.sql.catalyst.parser.ParseException: extraneous input '\' expecting <EOF>(line 2, pos 2) == SQL == SELECT concat('test', 'comment') -- someone's comment here ,2\ --^^^

Previously worked fine because of this very bug, the insideComment flag ignored everything until the end of the string. But the spark SQL parser does not recognize the backslashes. Line-continuity can be added to the CLI. But I think that feature should be added directly to the SQL parser to avoid confusion.

Let me know your thoughts 👍

If we can, the fix in SqlBase.g4 (SIMPLE_COMENT) looks fine to me and I think the queries above should work in Spark SQL: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4#L1811 Could you try?

@maropu I have added the fix. Let me know what you think :)

maropu · 2020-03-16T04:56:18Z

cc: @wangyum

maropu · 2020-04-09T08:16:57Z

@javierivanov Any update?

javierivanov · 2020-04-09T14:26:04Z

@maropu I am extremly sorry, I will commit soon :)

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala

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

SparkQA · 2020-04-13T07:05:01Z

Test build #121162 has finished for PR 27920 at commit 440dcbd.

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

maropu · 2020-04-13T07:11:39Z

retest this please

SparkQA · 2020-04-13T13:49:52Z

Test build #121181 has finished for PR 27920 at commit 440dcbd.

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

SparkQA · 2020-04-13T15:18:31Z

Test build #121211 has finished for PR 27920 at commit 0571f21.

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

maropu · 2020-04-14T01:39:08Z

retest this please

maropu · 2020-04-14T01:45:16Z

Could you check this? @wangyum

SparkQA · 2020-04-14T07:05:02Z

Test build #121243 has finished for PR 27920 at commit 0571f21.

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

maropu · 2020-04-14T07:12:20Z

retest this please

SparkQA · 2020-04-14T12:10:39Z

Test build #121260 has finished for PR 27920 at commit 0571f21.

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

javierivanov · 2020-05-06T14:58:33Z

ping @wangyum @maropu :)

maropu · 2020-05-06T23:50:21Z

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

@@ -1814,7 +1814,7 @@ fragment LETTER
    ;

 SIMPLE_COMMENT
-    : '--' ~[\r\n]* '\r'? '\n'? -> channel(HIDDEN)
+    : '--' ('\\\n' | ~[\r\n])* '\r'? '\n'? -> channel(HIDDEN)


Ur, one more comment; could you add tests in sql-tests/inputs/comments.sql, too?

maropu · 2020-05-06T23:58:19Z

retest this please

maropu · 2020-05-06T23:58:30Z

cc: @cloud-fan

cloud-fan · 2020-05-07T04:32:22Z

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala

    runCliWithin(1.minute)(
-      """SELECT concat('test', 'comment') -- someone's comment here \\


so double-slash doesn't work any more?

It was a previous mistake since using Scala multi-line strings it auto escape chars.

cloud-fan · 2020-05-07T04:33:55Z

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

    val plan = table("a").select(star())
    assertEqual("-- single comment\nSELECT * FROM a", plan)
  }

+  test("single comment case two") {
+    val plan = table("a").select(star())
+    assertEqual("-- single comment\\\nwith line continuity\nSELECT * FROM a", plan)


how to interpret \\\n? An escaped slash and a new-line symbol?

Thats correct. Inline strings need to be escaped.

SparkQA · 2020-05-07T05:04:41Z

Test build #122383 has finished for PR 27920 at commit 0571f21.

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

cloud-fan · 2020-05-12T13:46:17Z

thanks, merging to master!

cloud-fan · 2020-05-12T13:49:31Z

it conflicts with 3.0, @javierivanov can you open a new PR for 3.0? Thanks!

maropu · 2020-05-17T23:22:16Z

@javierivanov kindly ping: #27920 (comment)

This PR introduces a change to false for the insideComment flag on a newline. Fixing the issue introduced by SPARK-30049. Previously on SPARK-30049 a comment containing an unclosed quote produced the following issue: ``` spark-sql> SELECT 1 -- someone's comment here > ; Error in query: extraneous input ';' expecting <EOF>(line 2, pos 0) == SQL == SELECT 1 -- someone's comment here ; ^^^ ``` This was caused because there was no flag for comment sections inside the splitSemiColon method to ignore quotes. SPARK-30049 added that flag and fixed the issue, but introduced the follwoing problem: ``` spark-sql> select > 1, > -- two > 2; Error in query: mismatched input '<EOF>' expecting {'(', 'ADD', 'AFTER', 'ALL', 'ALTER', ...}(line 3, pos 2) == SQL == select 1, --^^^ ``` This issue is generated by a missing turn-off for the insideComment flag with a newline. No - For previous tests using line-continuity(`\`) it was added a line-continuity rule in the SqlBase.g4 file to add the functionality to the SQL context. - A new test for inline comments was added. Closes apache#27920 from javierivanov/SPARK-31102. Authored-by: Javier Fuentes <j.fuentes.m@icloud.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

This PR introduces a change to false for the insideComment flag on a newline. Fixing the issue introduced by SPARK-30049. Backport to 3.0 from #27920 Previously on SPARK-30049 a comment containing an unclosed quote produced the following issue: ``` spark-sql> SELECT 1 -- someone's comment here > ; Error in query: extraneous input ';' expecting <EOF>(line 2, pos 0) == SQL == SELECT 1 -- someone's comment here ; ^^^ ``` This was caused because there was no flag for comment sections inside the splitSemiColon method to ignore quotes. SPARK-30049 added that flag and fixed the issue, but introduced the follwoing problem: ``` spark-sql> select > 1, > -- two > 2; Error in query: mismatched input '<EOF>' expecting {'(', 'ADD', 'AFTER', 'ALL', 'ALTER', ...}(line 3, pos 2) == SQL == select 1, --^^^ ``` This issue is generated by a missing turn-off for the insideComment flag with a newline. No - For previous tests using line-continuity(`\`) it was added a line-continuity rule in the SqlBase.g4 file to add the functionality to the SQL context. - A new test for inline comments was added. Closes #27920 from javierivanov/SPARK-31102. Authored-by: Javier Fuentes <j.fuentes.micloud.com> Signed-off-by: Wenchen Fan <wenchendatabricks.com> ### 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? Closes #28565 from javierivanov/SPARK-3.0-31102. Authored-by: Javier Fuentes <j.fuentes.m@icloud.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

This PR introduces a change to false for the insideComment flag on a newline. Fixing the issue introduced by SPARK-30049. Previously on SPARK-30049 a comment containing an unclosed quote produced the following issue: ``` spark-sql> SELECT 1 -- someone's comment here > ; Error in query: extraneous input ';' expecting <EOF>(line 2, pos 0) == SQL == SELECT 1 -- someone's comment here ; ^^^ ``` This was caused because there was no flag for comment sections inside the splitSemiColon method to ignore quotes. SPARK-30049 added that flag and fixed the issue, but introduced the follwoing problem: ``` spark-sql> select > 1, > -- two > 2; Error in query: mismatched input '<EOF>' expecting {'(', 'ADD', 'AFTER', 'ALL', 'ALTER', ...}(line 3, pos 2) == SQL == select 1, --^^^ ``` This issue is generated by a missing turn-off for the insideComment flag with a newline. No - For previous tests using line-continuity(`\`) it was added a line-continuity rule in the SqlBase.g4 file to add the functionality to the SQL context. - A new test for inline comments was added. Closes apache#27920 from javierivanov/SPARK-31102. Authored-by: Javier Fuentes <j.fuentes.m@icloud.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

javierivanov added 2 commits March 15, 2020 22:19

fixes for SPARK-31102

5c0a000

fixed tests

d69d271

maropu reviewed Mar 16, 2020

View reviewed changes

javierivanov added 2 commits April 12, 2020 13:46

Merge branch 'master' into SPARK-31102

9d354c2

added multi-line to single comments

440dcbd

probot-autolabeler bot added the SQL label Apr 13, 2020

maropu reviewed Apr 13, 2020

View reviewed changes

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala Outdated Show resolved Hide resolved

maropu reviewed Apr 13, 2020

View reviewed changes

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala Outdated Show resolved Hide resolved

maropu reviewed Apr 13, 2020

View reviewed changes

sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala Outdated Show resolved Hide resolved

maropu reviewed Apr 13, 2020

View reviewed changes

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

fixes

0571f21

maropu reviewed May 6, 2020

View reviewed changes

cloud-fan reviewed May 7, 2020

View reviewed changes

cloud-fan closed this in 178ca96 May 12, 2020

javierivanov mentioned this pull request May 18, 2020

[SPARK-31102][SQL][3.0] Spark-sql fails to parse when contains comment #28565

Closed

turboFei mentioned this pull request Jan 5, 2021

[SPARK-33100][SQL][3.0] Ignore a semicolon inside a bracketed comment in spark-sql #31033

Closed

turboFei mentioned this pull request Jan 5, 2021

[SPARK-33100][SQL][2.4] Ignore a semicolon inside a bracketed comment in spark-sql #31040

Closed

		runCliWithin(1.minute)(
		"""SELECT concat('test', 'comment') -- someone's comment here \\

[SPARK-31102][SQL] Spark-sql fails to parse when contains comment. #27920

[SPARK-31102][SQL] Spark-sql fails to parse when contains comment. #27920

Uh oh!

Conversation

javierivanov commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

maropu commented Mar 16, 2020

Uh oh!

SparkQA commented Mar 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu Mar 17, 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!

maropu commented Mar 16, 2020

Uh oh!

maropu commented Apr 9, 2020

Uh oh!

javierivanov commented Apr 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

maropu commented Apr 13, 2020

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

maropu commented Apr 14, 2020

Uh oh!

maropu commented Apr 14, 2020

Uh oh!

SparkQA commented Apr 14, 2020

Uh oh!

maropu commented Apr 14, 2020

Uh oh!

SparkQA commented Apr 14, 2020

Uh oh!

javierivanov commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented May 6, 2020

Uh oh!

maropu commented May 6, 2020

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

Uh oh!

cloud-fan commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented May 12, 2020

Uh oh!

maropu commented May 17, 2020

Uh oh!

Uh oh!

javierivanov commented Mar 16, 2020 •

edited

Loading

maropu Mar 17, 2020 •

edited

Loading

cloud-fan commented May 12, 2020 •

edited

Loading