[SPARK-33100][SQL][FOLLOWUP] Find correct bound of bracketed comment in spark-sql by turboFei · Pull Request #31054 · apache/spark

turboFei · 2021-01-06T04:28:02Z

What changes were proposed in this pull request?

This PR help find correct bound of bracketed comment in spark-sql.

Here is the log for UT of SPARK-33100 in CliSuite before:

2021-01-05 13:22:34.768 - stdout> spark-sql> /* SELECT 'test';*/ SELECT 'test';
2021-01-05 13:22:41.523 - stderr> Time taken: 6.716 seconds, Fetched 1 row(s)
2021-01-05 13:22:41.599 - stdout> test
2021-01-05 13:22:41.6 - stdout> spark-sql> ;;/* SELECT 'test';*/ SELECT 'test';
2021-01-05 13:22:41.709 - stdout> test
2021-01-05 13:22:41.709 - stdout> spark-sql> /* SELECT 'test';*/;; SELECT 'test';
2021-01-05 13:22:41.902 - stdout> spark-sql> SELECT 'test'; -- SELECT 'test';
2021-01-05 13:22:41.902 - stderr> Time taken: 0.129 seconds, Fetched 1 row(s)
2021-01-05 13:22:41.902 - stderr> Error in query: 2021-01-05 13:22:41.902 - stderr> 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 19)
2021-01-05 13:22:42.006 - stderr> 2021-01-05 13:22:42.006 - stderr> == SQL ==
2021-01-05 13:22:42.006 - stderr> /* SELECT 'test';*/
2021-01-05 13:22:42.006 - stderr> -------------------^^^
2021-01-05 13:22:42.006 - stderr> 2021-01-05 13:22:42.006 - stderr> Time taken: 0.226 seconds, Fetched 1 row(s)
2021-01-05 13:22:42.006 - stdout> test

The root cause is that the insideBracketedComment is not accurate.

For /* comment */, the last character / is not insideBracketedComment and it would be treat as beginning of statements.

In this PR, this issue is fixed.

Why are the changes needed?

To fix the issue described above.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UT

turboFei · 2021-01-06T04:34:55Z

After this PR, the log in sql/hive-thriftserver/target/unit-tests.log

12:23:18.249 Thread-6 INFO CliSuite: 2021-01-05 20:23:18.249 - stdout> spark-sql> /* SELECT 'test';*/ SELECT 'test';
12:23:18.249 Thread-6 INFO CliSuite: stdout> found expected output line 0: 'spark-sql> /* SELECT 'test';*/ SELECT 'test';'
12:23:22.844 Thread-6 INFO CliSuite: 2021-01-05 20:23:22.844 - stdout> test
12:23:22.844 Thread-6 INFO CliSuite: stdout> found expected output line 1: 'test'
12:23:22.844 Thread-7 INFO CliSuite: 2021-01-05 20:23:22.844 - stderr> Time taken: 4.573 seconds, Fetched 1 row(s)
12:23:22.867 Thread-6 INFO CliSuite: 2021-01-05 20:23:22.867 - stdout> spark-sql> ;;/* SELECT 'test';*/ SELECT 'test';
12:23:22.868 Thread-6 INFO CliSuite: stdout> found expected output line 2: 'spark-sql> ;;/* SELECT 'test';*/ SELECT 'test';'
12:23:22.928 Thread-6 INFO CliSuite: 2021-01-05 20:23:22.927 - stdout> test
12:23:22.928 Thread-6 INFO CliSuite: stdout> found expected output line 3: 'test'
12:23:22.928 Thread-7 INFO CliSuite: 2021-01-05 20:23:22.928 - stderr> Time taken: 0.059 seconds, Fetched 1 row(s)
12:23:22.941 Thread-6 INFO CliSuite: 2021-01-05 20:23:22.941 - stdout> spark-sql> /* SELECT 'test';*/;; SELECT 'test';
12:23:22.941 Thread-6 INFO CliSuite: stdout> found expected output line 4: 'spark-sql> /* SELECT 'test';*/;; SELECT 'test';'
12:23:22.993 Thread-6 INFO CliSuite: 2021-01-05 20:23:22.993 - stdout> test
12:23:22.994 Thread-6 INFO CliSuite: stdout> found expected output line 5: 'test'
12:23:22.994 Thread-7 INFO CliSuite: 2021-01-05 20:23:22.994 - stderr> Time taken: 0.052 seconds, Fetched 1 row(s)
12:23:23.007 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.007 - stdout> spark-sql> SELECT 'test'; -- SELECT 'test';
12:23:23.007 Thread-6 INFO CliSuite: stdout> found expected output line 6: 'spark-sql> SELECT 'test'; -- SELECT 'test';'
12:23:23.067 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.067 - stdout> test
12:23:23.067 Thread-6 INFO CliSuite: stdout> found expected output line 7: ''
12:23:23.067 Thread-7 INFO CliSuite: 2021-01-05 20:23:23.067 - stderr> Time taken: 0.059 seconds, Fetched 1 row(s)
12:23:23.083 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.083 - stdout> spark-sql> SELECT 'test'; /* SELECT 'test';*/;
12:23:23.083 Thread-6 INFO CliSuite: stdout> found expected output line 8: 'spark-sql> SELECT 'test'; /* SELECT 'test';*/;'
12:23:23.130 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.13 - stdout> test
12:23:23.130 Thread-6 INFO CliSuite: stdout> found expected output line 9: ''
12:23:23.130 Thread-7 INFO CliSuite: 2021-01-05 20:23:23.13 - stderr> Time taken: 0.046 seconds, Fetched 1 row(s)
12:23:23.144 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.144 - stdout> spark-sql> /*$meta chars{^\;}*/ SELECT 'test';
12:23:23.144 Thread-6 INFO CliSuite: stdout> found expected output line 10: 'spark-sql> /*$meta chars{^\;}*/ SELECT 'test';'
12:23:23.191 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.191 - stdout> test
12:23:23.191 Thread-6 INFO CliSuite: stdout> found expected output line 11: 'test'
12:23:23.191 Thread-7 INFO CliSuite: 2021-01-05 20:23:23.191 - stderr> Time taken: 0.046 seconds, Fetched 1 row(s)
12:23:23.202 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.202 - stdout> spark-sql> /*
12:23:23.202 Thread-6 INFO CliSuite: stdout> found expected output line 12: 'spark-sql> /*'
12:23:23.215 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.215 - stdout> > multi-line
12:23:23.215 Thread-6 INFO CliSuite: stdout> found expected output line 13: ' > multi-line'
12:23:23.228 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.228 - stdout> > */ SELECT 'test';
12:23:23.228 Thread-6 INFO CliSuite: stdout> found expected output line 14: ' > */ SELECT 'test';'
12:23:23.271 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.271 - stdout> test
12:23:23.271 Thread-6 INFO CliSuite: stdout> found expected output line 15: 'test'
12:23:23.271 Thread-7 INFO CliSuite: 2021-01-05 20:23:23.271 - stderr> Time taken: 0.042 seconds, Fetched 1 row(s)
12:23:23.290 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.29 - stdout> spark-sql> /*/* multi-level bracketed*/ SELECT 'test';
12:23:23.291 Thread-6 INFO CliSuite: stdout> found expected output line 16: 'spark-sql> /*/* multi-level bracketed*/ SELECT 'test';'
12:23:23.335 Thread-6 INFO CliSuite: 2021-01-05 20:23:23.335 - stdout> test
12:23:23.335 Thread-6 INFO CliSuite: stdout> found expected output line 17: 'test'

…park-sql

turboFei · 2021-01-06T04:40:47Z

cc @maropu @yaooqinn @dongjoon-hyun @gatorsmile

maropu · 2021-01-06T12:02:04Z

ok to test

maropu · 2021-01-06T12:05:15Z

also cc: @bogdanghit

maropu · 2021-01-06T12:06:35Z

For /* comment */, the last character / is not insideBracketedComment and it would be treat as beginning of statements.

This causes the flakiness? Why can the test pass sometimes?

SparkQA · 2021-01-06T12:55:29Z

Test build #133738 has finished for PR 31054 at commit b42d28f.

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

SparkQA · 2021-01-06T12:59:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38326/

turboFei · 2021-01-06T13:11:31Z

according to the log you attached(https://github.com/apache/spark/pull/31045/checks?check_run_id=1652972350),
we can see that there are two continuous query lines, it is abnormal.

Is it possible that, for the CliSuite, we launch a process for test and write all queries into the outputstream of this process,
might the process stuck(might caused by the issue fixed by this pr?) some moment, and two continuous lines inputed and then only one result returned at last.

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

Lines 167 to 172 in 6788304

valprocess=newProcessBuilder(command: _*).start() 
valstdinWriter=newOutputStreamWriter(process.getOutputStream, StandardCharsets.UTF_8) 
 stdinWriter.write(queriesString) 
 stdinWriter.flush() 
 stdinWriter.close() 

So one result loss and can not be matched, so the UT failed at last.

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

Line 138 in 6788304

defcaptureOutput(source: String)(line: String):Unit= lock.synchronized {

Is it better to change it to?

 val stdinWriter = new OutputStreamWriter(process.getOutputStream, StandardCharsets.UTF_8)
queryLineList.foreach { queryLine =>
stdinWriter.write(queryLine)
stdinWriter.flush()
}
stdinWriter.close()

SparkQA · 2021-01-06T13:30:24Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38326/

SparkQA · 2021-01-06T13:57:50Z

Test build #133744 has finished for PR 31054 at commit a317344.

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

maropu · 2021-01-07T01:36:50Z

Is it better to change it to?

If we flushed it for each query, did we could notice this issue right away? If yes, I think it is worth doing so. The change could make the test running time of `CliSuite longer? Could you check it in a separate PR after this issue fixed?

turboFei · 2021-01-07T02:58:41Z

If we flushed it for each query, did we could notice this issue right away? If yes, I think it is worth doing so. The change could make the test running time of `CliSuite longer? Could you check it in a separate PR after this issue fixed?

I think it would not cost much time, will try it later.

turboFei · 2021-01-07T03:34:04Z

Here is a test case(query contains '\n') which can not be covered by runCliWithin:

 runCliWithin(1.minute)("select 'test1';\n select 'test2';" -> "test2")

log:

11:35:00.383 pool-1-thread-1-ScalaTest-running-CliSuite INFO CliSuite: Cli driver is booted. Waiting for expected answers.
11:35:01.104 Thread-6 INFO CliSuite: 2021-01-06 19:35:01.104 - stdout> spark-sql> select 'test1';
11:35:01.104 Thread-6 INFO CliSuite: stdout> found expected output line 0: 'spark-sql> select 'test1';'
11:35:10.120 Thread-6 INFO CliSuite: 2021-01-06 19:35:10.12 - stdout> test1
11:35:10.121 Thread-7 INFO CliSuite: 2021-01-06 19:35:10.121 - stderr> Time taken: 8.987 seconds, Fetched 1 row(s)
11:35:10.151 Thread-6 INFO CliSuite: 2021-01-06 19:35:10.151 - stdout> spark-sql> select 'test2';
11:35:10.220 Thread-6 INFO CliSuite: 2021-01-06 19:35:10.22 - stdout> test2
11:35:10.220 Thread-7 INFO CliSuite: 2021-01-06 19:35:10.22 - stderr> Time taken: 0.068 seconds, Fetched 1 row(s)
11:35:10.443 Thread-6 INFO CliSuite: 2021-01-06 19:35:10.443 - stdout> spark-sql> 11:36:00.390 pool-1-thread-1-ScalaTest-running-CliSuite ERROR CliSuite: =======================
CliSuite failure output
=======================
Spark SQL CLI command line: ../../bin/spark-sql --master local --driver-java-options -Dderby.system.durability=test --conf spark.ui.enabled=false --hiveconf javax.jdo.option.ConnectionURL=jdbc:derby:;databaseName=/Users/fwang12/ebay/apache-spark/target/tmp/spark-7c275c0c-fc8e-49c7-b643-18f20fe8ba51;create=true --hiveconf hive.exec.scratchdir=/Users/fwang12/ebay/apache-spark/target/tmp/spark-bb0fded2-1a25-45d2-9f78-16898f32aefc --hiveconf conf1=conftest --hiveconf conf2=1 --hiveconf hive.metastore.warehouse.dir=/Users/fwang12/ebay/apache-spark/target/tmp/spark-4901396f-9a7a-4299-b7fc-9cb3b24c46f4
Exception: java.util.concurrent.TimeoutException: Futures timed out after [1 minute]
Failed to capture next expected output " > select 'test2';" within 1 minute.

It seems that it is not better to transfer multiple queries one time, but there is some UT like this:

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

Lines 542 to 544 in f9daf03

 test("SPARK-31595 Should allow unescaped quote mark in quoted string") { 
 runCliWithin(1.minute)( 
"SELECT '\"legal string a';select 1 + 234;".stripMargin ->"235"

SparkQA · 2021-01-07T03:55:40Z

Test build #133766 has finished for PR 31054 at commit ada4d5c.

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

SparkQA · 2021-01-07T03:59:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38353/

maropu

Looks fine. To check if we can fix the flakiness, could you push empty commits for invoking GA tests three or four times?

maropu · 2021-01-07T04:30:08Z

Here is a test case(query contains '\n') which can not be covered by runCliWithin:

Thanks for investigating it , @turboFei . Could you file another jira and make a PR for it?

SparkQA · 2021-01-07T04:34:53Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38353/

turboFei · 2021-01-07T05:55:28Z

Could you file another jira and make a PR for it?

Sure.

SparkQA · 2021-01-07T06:54:34Z

Test build #133777 has finished for PR 31054 at commit e1814a6.

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

SparkQA · 2021-01-07T07:00:02Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38366/

maropu · 2021-01-07T07:11:33Z

To check if we can fix the flakiness, could you push empty commits for invoking GA tests three or four times?

@turboFei Could you do this?

SparkQA · 2021-01-07T07:29:40Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38366/

turboFei · 2021-01-07T07:33:50Z

To check if we can fix the flakiness, could you push empty commits for invoking GA tests three or four times?

ok, I will trigger GA test 3 times again.

maropu · 2021-01-07T07:49:11Z

@turboFei You need to push two more empty commits for invoking GA tests concurrently. It seems that only one test job is running now.

maropu · 2021-01-07T07:56:43Z

Thanks for it! Let's wait for the test results...

SparkQA · 2021-01-07T08:51:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38371/

SparkQA · 2021-01-07T08:53:47Z

Test build #133783 has finished for PR 31054 at commit d348489.

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

SparkQA · 2021-01-07T08:56:11Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38371/

turboFei · 2021-01-07T09:28:02Z

For the first time GA test:

[error] Failed tests:
[error] org.apache.spark.sql.execution.datasources.orc.OrcEncryptionSuite

maropu · 2021-01-07T09:43:15Z

For the first time GA test:
[error] Failed tests:
[error] org.apache.spark.sql.execution.datasources.orc.OrcEncryptionSuite

Yea, that's the known issue.

turboFei · 2021-01-07T11:11:46Z

the second and third succeed.

maropu · 2021-01-07T11:47:09Z

okay, thanks for the quick fix! I'll merge this to fix the flakiness. FYI: @HyukjinKwon @dongjoon-hyun If you have any further comments, feel free to leave comments here.

…in spark-sql ### What changes were proposed in this pull request? This PR help find correct bound of bracketed comment in spark-sql. Here is the log for UT of SPARK-33100 in CliSuite before: ``` 2021-01-05 13:22:34.768 - stdout> spark-sql> /* SELECT 'test';*/ SELECT 'test'; 2021-01-05 13:22:41.523 - stderr> Time taken: 6.716 seconds, Fetched 1 row(s) 2021-01-05 13:22:41.599 - stdout> test 2021-01-05 13:22:41.6 - stdout> spark-sql> ;;/* SELECT 'test';*/ SELECT 'test'; 2021-01-05 13:22:41.709 - stdout> test 2021-01-05 13:22:41.709 - stdout> spark-sql> /* SELECT 'test';*/;; SELECT 'test'; 2021-01-05 13:22:41.902 - stdout> spark-sql> SELECT 'test'; -- SELECT 'test'; 2021-01-05 13:22:41.902 - stderr> Time taken: 0.129 seconds, Fetched 1 row(s) 2021-01-05 13:22:41.902 - stderr> Error in query: 2021-01-05 13:22:41.902 - stderr> 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 19) 2021-01-05 13:22:42.006 - stderr> 2021-01-05 13:22:42.006 - stderr> == SQL == 2021-01-05 13:22:42.006 - stderr> /* SELECT 'test';*/ 2021-01-05 13:22:42.006 - stderr> -------------------^^^ 2021-01-05 13:22:42.006 - stderr> 2021-01-05 13:22:42.006 - stderr> Time taken: 0.226 seconds, Fetched 1 row(s) 2021-01-05 13:22:42.006 - stdout> test ``` The root cause is that the insideBracketedComment is not accurate. For `/* comment */`, the last character `/` is not insideBracketedComment and it would be treat as beginning of statements. In this PR, this issue is fixed. ### Why are the changes needed? To fix the issue described above. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing UT Closes#31054 from turboFei/SPARK-33100-followup. Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org> (cherry picked from commit 7b06acc) Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

maropu · 2021-01-07T11:50:32Z

Merged to master/branch-3.1.

turboFei changed the title ~~[SPARK-33100][FOLLOWUP] Find right bound of bracketed comment in spark-sql~~[SPARK-33100][FOLLOWUP] Find correct bound of bracketed comment in spark-sqlJan 6, 2021

turboFei mentioned this pull request Jan 6, 2021
[SPARK-33100][SQL] Ignore a semicolon inside a bracketed comment in spark-sql #29982
Closed

[SPARK-33100][FOLLOWUP] Find correct bound for bracketed comment in s…
b42d28f
…park-sql

github-actionsBot added the SQL label Jan 6, 2021

maropu changed the title ~~[SPARK-33100][FOLLOWUP] Find correct bound of bracketed comment in spark-sql~~[SPARK-33100][SQL][FOLLOWUP] Find correct bound of bracketed comment in spark-sqlJan 6, 2021

complete ut
a317344