Skip to content
Closed
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1797,11 +1797,11 @@ SIMPLE_COMMENT
;

BRACKETED_EMPTY_COMMENT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we still need it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No need.

: '/**/' -> channel(HIDDEN)
: '/*' BRACKETED_EMPTY_COMMENT? '*/' -> channel(HIDDEN)
;

BRACKETED_COMMENT
Comment thread
cloud-fan marked this conversation as resolved.
: '/*' ~[+] .*? '*/' -> channel(HIDDEN)
: '/*' ~[+] (BRACKETED_COMMENT|.)*? '*/' -> channel(HIDDEN)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It will fail

/*/**/*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually does anyone know why there is ~[+] here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For hints?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@maropu ah, right

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yea!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we create a lexer rule for hint and put it before this one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gengliangwang Now, it works well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan I fixed the issue @gengliangwang proposed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about the @cloud-fan comment? The fix looks nice if we can. Its okay to fix it in followup though.

@gengliangwang gengliangwang Feb 18, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@beliefer my point of the case

/*/**/*/

is not about empty comment. It is about matching the begining of another comment ( /* after /*).
We can change the case to

/*/*foo*/*/

The latest fix will still failed.

;

WS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,86 @@ class PlanParserSuite extends AnalysisTest {
With(plan, ctes)
}

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

test("bracketed comment case one") {
val plan = table("a").select(star())
assertEqual(
"""
|/* This is an example of SQL which should not execute:
| * select 'multi-line';
| */
|SELECT * FROM a
""".stripMargin, plan)
}

test("bracketed comment case two") {
val plan = table("a").select(star())
assertEqual(
"""
|/*
|SELECT 'trailing' as x1; -- inside block comment
|*/
|SELECT * FROM a
""".stripMargin, plan)
}

test("nested bracketed comment case one") {
val plan = table("a").select(star())
assertEqual(
"""
|/* This block comment surrounds a query which itself has a block comment...
|SELECT /* embedded single line */ 'embedded' AS x2;
|*/
|SELECT * FROM a
""".stripMargin, plan)
}

test("nested bracketed comment case two") {
val plan = table("a").select(star())
assertEqual(
"""
|SELECT -- continued after the following block comments...
|/* Deeply nested comment.
| This includes a single apostrophe to make sure we aren't decoding this part as a string.
|SELECT 'deep nest' AS n1;
|/* Second level of nesting...
|SELECT 'deeper nest' as n2;
|/* Third level of nesting...
|SELECT 'deepest nest' as n3;
|*/
|Hoo boy. Still two deep...
|*/
|Now just one deep...
|*/
|* FROM a
""".stripMargin, plan)
}

test("nested bracketed comment case three") {
val plan = table("a").select(star())
assertEqual(
"""
|/* This block comment surrounds a query which itself has a block comment...
|//* I am a nested bracketed comment.
|*/
|*/
|SELECT * FROM a
""".stripMargin, plan)
}

test("nested bracketed comment case four") {
val plan = table("a").select(star())
assertEqual(
"""
|/*/**/*/
|SELECT * FROM a
""".stripMargin, plan)
}
Comment thread
cloud-fan marked this conversation as resolved.

test("case insensitive") {
val plan = table("a").select(star())
assertEqual("sELEct * FroM a", plan)
Expand Down
90 changes: 90 additions & 0 deletions sql/core/src/test/resources/sql-tests/inputs/comments.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
-- Test 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 second case of bracketed comment
--QUERY-DELIMITER-START
/* This is the second example of bracketed comment.
SELECT '/', 'ommented out content' AS second;
*/
SELECT '/', 'selected content' AS second;
--QUERY-DELIMITER-END

-- the third case of bracketed comment
--QUERY-DELIMITER-START
/* This is the third example of bracketed comment.
*SELECT '*', 'ommented out content' AS third;
*/
SELECT '*', 'selected content' AS third;
--QUERY-DELIMITER-END

-- the first case of empty bracketed comment
--QUERY-DELIMITER-START
/**/
SELECT 'selected content' AS fourth;
--QUERY-DELIMITER-END

-- the first case of nested bracketed comment
--QUERY-DELIMITER-START
/* This is the first example of nested bracketed comment.
/* I am a nested bracketed comment.*/
*/

@cloud-fan cloud-fan Feb 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOL even github can't highlight the nested bracketed comment correctly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Haha

SELECT 'selected content' AS fifth;
--QUERY-DELIMITER-END

-- the second case of nested bracketed comment
--QUERY-DELIMITER-START
/* This is the second example of nested bracketed comment.
/* I am a nested bracketed comment.
*/
*/
SELECT 'selected content' AS sixth;
--QUERY-DELIMITER-END

-- the third case of nested bracketed comment
--QUERY-DELIMITER-START
/*
* This is the third example of nested bracketed comment.
/*
* I am a nested bracketed comment.
*/
*/
SELECT 'selected content' AS seventh;
--QUERY-DELIMITER-END

-- the fourth case of nested bracketed comment
--QUERY-DELIMITER-START
/*
* This is the fourth example of nested bracketed comment.
SELECT /* I am a nested bracketed comment.*/ * FROM testData;
*/
SELECT 'selected content' AS eighth;
--QUERY-DELIMITER-END

-- the fifth case of nested bracketed comment
--QUERY-DELIMITER-START
SELECT /*
* This is the fifth example of nested bracketed comment.
/* I am a second level of nested bracketed comment.
/* I am a third level of nested bracketed comment.
Other information of third level.
SELECT 'ommented out content' AS ninth;
*/
Other information of second level.
*/
Other information of first level.
*/
'selected content' AS ninth;
--QUERY-DELIMITER-END

-- the first case of empty nested bracketed comment
--QUERY-DELIMITER-START
/*/**/*/
SELECT 'selected content' AS tenth;
--QUERY-DELIMITER-END
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ SELECT /* both embedded and trailing single line */ 'both' AS third; -- trailing

SELECT 'before multi-line' AS fourth;
--QUERY-DELIMITER-START
-- [SPARK-28880] ANSI SQL: Bracketed comments
/* This is an example of SQL which should not execute:
* select 'multi-line';
*/
SELECT 'after multi-line' AS fifth;
--QUERY-DELIMITER-END

-- [SPARK-28880] ANSI SQL: Bracketed comments
--
-- Nested comments
--
Expand Down Expand Up @@ -47,4 +45,5 @@ Now just one deep...
*/
'deeply nested example' AS sixth;
--QUERY-DELIMITER-END
/* and this is the end of the file */
-- [SPARK-30824] Support submit sql content only contains comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this an ANSI-related issue? https://issues.apache.org/jira/browse/SPARK-30824

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. But postgreSQL test support it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see. If so, can you move the parent ticket to https://issues.apache.org/jira/browse/SPARK-30375 ? I think this feature is implementation-dependent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK.

-- /* and this is the end of the file */
121 changes: 121 additions & 0 deletions sql/core/src/test/resources/sql-tests/results/comments.sql.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 10


-- !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


-- !query
/* This is the second example of bracketed comment.
SELECT '/', 'ommented out content' AS second;
*/
SELECT '/', 'selected content' AS second
-- !query schema
struct</:string,second:string>
-- !query output
/ selected content


-- !query
/* This is the third example of bracketed comment.
*SELECT '*', 'ommented out content' AS third;
*/
SELECT '*', 'selected content' AS third
-- !query schema
struct<*:string,third:string>
-- !query output
* selected content


-- !query
/**/
SELECT 'selected content' AS fourth
-- !query schema
struct<fourth:string>
-- !query output
selected content


-- !query
/* This is the first example of nested bracketed comment.
/* I am a nested bracketed comment.*/
*/
SELECT 'selected content' AS fifth
-- !query schema
struct<fifth:string>
-- !query output
selected content


-- !query
/* This is the second example of nested bracketed comment.
/* I am a nested bracketed comment.
*/
*/
SELECT 'selected content' AS sixth
-- !query schema
struct<sixth:string>
-- !query output
selected content


-- !query
/*
* This is the third example of nested bracketed comment.
/*
* I am a nested bracketed comment.
*/
*/
SELECT 'selected content' AS seventh
-- !query schema
struct<seventh:string>
-- !query output
selected content


-- !query
/*
* This is the fourth example of nested bracketed comment.
SELECT /* I am a nested bracketed comment.*/ * FROM testData;
*/
SELECT 'selected content' AS eighth
-- !query schema
struct<eighth:string>
-- !query output
selected content


-- !query
SELECT /*
* This is the fifth example of nested bracketed comment.
/* I am a second level of nested bracketed comment.
/* I am a third level of nested bracketed comment.
Other information of third level.
SELECT 'ommented out content' AS ninth;
*/
Other information of second level.
*/
Other information of first level.
*/
'selected content' AS ninth
-- !query schema
struct<ninth:string>
-- !query output
selected content


-- !query
/*/**/*/
SELECT 'selected content' AS tenth
-- !query schema
struct<tenth:string>
-- !query output
selected content
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 7
-- Number of queries: 6


-- !query
Expand Down Expand Up @@ -69,47 +69,6 @@ Now just one deep...
*/
'deeply nested example' AS sixth
-- !query schema
struct<>
struct<sixth:string>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException

mismatched input ''embedded'' 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 6, pos 34)

== SQL ==
/*
SELECT 'trailing' as x1; -- inside block comment
*/

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

SELECT -- continued after the following block comments...
/* Deeply nested comment.
This includes a single apostrophe to make sure we aren't decoding this part as a string.
SELECT 'deep nest' AS n1;
/* Second level of nesting...
SELECT 'deeper nest' as n2;
/* Third level of nesting...
SELECT 'deepest nest' as n3;
*/
Hoo boy. Still two deep...
*/
Now just one deep...
*/
'deeply nested example' AS sixth


-- !query
/* and this is the end of the file */
-- !query schema
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException

mismatched input '<EOF>' 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 37)

== SQL ==
/* and this is the end of the file */
-------------------------------------^^^
deeply nested example