Skip to content

[SPARK-31406][SQL][TEST] ThriftServerQueryTestSuite: Sharing test data and test tables among multiple test cases. - #28180

Closed
beliefer wants to merge 1 commit into
apache:masterfrom
beliefer:avoid-load-thrift-test-data-repeatedly
Closed

[SPARK-31406][SQL][TEST] ThriftServerQueryTestSuite: Sharing test data and test tables among multiple test cases.#28180
beliefer wants to merge 1 commit into
apache:masterfrom
beliefer:avoid-load-thrift-test-data-repeatedly

Conversation

@beliefer

@belieferbeliefer commented Apr 10, 2020

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR is related to #28060.
ThriftServerQueryTestSuite spend 17 minutes time to test.
I checked the code and found ThriftServerQueryTestSuite load test data repeatedly.
I've listed all the test cases order by time with desc in the hive-thriftserver module below.

ClassSpend time ↑FailureSkipPassTotal test case
ThriftServerQueryTestSuite17 minutes015140155
CliSuite8 minutes 24 seconds002424
SparkThriftServerProtocolVersionsSuite59 seconds00210210
HiveThriftBinaryServerSuite36 seconds012122
SparkMetadataOperationSuite19 seconds0077
HiveCliSessionStateSuite16 seconds0022
SparkSQLEnvSuite16 seconds0011
HiveThriftHttpServerSuite15 seconds0033
SingleSessionSuite14 seconds0033
JdbcConnectionUriSuite2.1 seconds0011
ThriftServerWithSparkContextSuite1.4 seconds0011
SparkExecuteStatementOperationSuite63 millseconds0022
UISeleniumSuite-1 millseconds0101

I checked the code of ThriftServerQueryTestSuite and found ThriftServerQueryTestSuite load test data repeatedly.
This PR will improve the performance of ThriftServerQueryTestSuite.
Because #28060 provides createTestTables(

privatedefcreateTestTables(session: SparkSession):Unit= {
) and removeTestTables(
privatedefremoveTestTables(session: SparkSession):Unit= {
), this PR will still uses them.
The total time run ThriftServerQueryTestSuite before and after this PR show below.
Before

NoTime
118 minutes, 8 seconds
222 minutes, 44 seconds
317 minutes, 48 seconds
418 minutes, 30 seconds

After

NoTime
116 minutes, 11 seconds
217 minutes, 19 seconds
318 minutes, 15 seconds
417 minutes, 27 seconds

Why are the changes needed?

Improve the performance of ThriftServerQueryTestSuite.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

Jenkins test

@SparkQA

Copy link
Copy Markdown

Test build #121068 has finished for PR 28180 at commit f2a3798.

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

@beliefer

Copy link
Copy Markdown
ContributorAuthor

cc @cloud-fan@maropu

@cloud-fan

Copy link
Copy Markdown
Contributor

thanks, merging to master/3.0!

cloud-fan pushed a commit that referenced this pull request Apr 10, 2020
…a and test tables among multiple test cases
### What changes were proposed in this pull request?
This PR is related to #28060.
`ThriftServerQueryTestSuite` spend 17 minutes time to test.
I checked the code and found `ThriftServerQueryTestSuite` load test data repeatedly.
I've listed all the test cases order by time with desc in the `hive-thriftserver` module below.
Class | Spend time ↑ | Failure | Skip | Pass | Total test case
-- | -- | -- | -- | -- | --
ThriftServerQueryTestSuite | 17 minutes | 0 | 15 | 140 | 155
CliSuite | 8 minutes 24 seconds | 0 | 0 | 24 | 24
SparkThriftServerProtocolVersionsSuite | 59 seconds | 0 | 0 | 210 | 210
HiveThriftBinaryServerSuite | 36 seconds | 0 | 1 | 21 | 22
SparkMetadataOperationSuite | 19 seconds | 0 | 0 | 7 | 7
HiveCliSessionStateSuite | 16 seconds | 0 | 0 | 2 | 2
SparkSQLEnvSuite | 16 seconds | 0 | 0 | 1 | 1
HiveThriftHttpServerSuite | 15 seconds | 0 | 0 | 3 | 3
SingleSessionSuite | 14 seconds | 0 | 0 | 3 | 3
JdbcConnectionUriSuite | 2.1 seconds | 0 | 0 | 1 | 1
ThriftServerWithSparkContextSuite | 1.4 seconds | 0 | 0 | 1 | 1
SparkExecuteStatementOperationSuite | 63 millseconds | 0 | 0 | 2 | 2
UISeleniumSuite | -1 millseconds | 0 | 1 | 0 | 1
I checked the code of `ThriftServerQueryTestSuite` and found `ThriftServerQueryTestSuite` load test data repeatedly.
This PR will improve the performance of `ThriftServerQueryTestSuite`.
Because #28060 provides `createTestTables`(https://github.com/apache/spark/blob/e42a3945acd614a26c7941a9eed161b500fb4520/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L574) and `removeTestTables`(https://github.com/apache/spark/blob/e42a3945acd614a26c7941a9eed161b500fb4520/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L666), this PR will still uses them.
The total time run `ThriftServerQueryTestSuite` before and after this PR show below.
Before
No | Time
-- | --
1 | 18 minutes, 8 seconds
2 | 22 minutes, 44 seconds
3 | 17 minutes, 48 seconds
4 | 18 minutes, 30 seconds
After
No | Time
-- | --
1 | 16 minutes, 11 seconds
2 | 17 minutes, 19 seconds
3 | 18 minutes, 15 seconds
4 | 17 minutes, 27 seconds
### Why are the changes needed?
Improve the performance of `ThriftServerQueryTestSuite`.
### Does this PR introduce any user-facing change?
'No'.
### How was this patch tested?
Jenkins test
Closes#28180 from beliefer/avoid-load-thrift-test-data-repeatedly.
Authored-by: beliefer <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 2d3692e)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@beliefer

Copy link
Copy Markdown
ContributorAuthor

@cloud-fan Thank you.

@maropu

Copy link
Copy Markdown
Member

late LGTM

@dongjoon-hyun

Copy link
Copy Markdown
Member

Hi, All.
Unfortunately, the original SQLQueryTestSuite commit broke this ThriftServerQueryTestSuite in Maven environment in master and branch-3.0.

Please see the follow-up fix.

@juliuszsompolskijuliuszsompolski left a 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.

@cloud-fan@beliefer could we do a followup for this suite to keep loading the data via JDBC? I think it was important part of the coverage.

"""
|CREATE OR REPLACE TEMPORARY VIEW testdata as
|SELECT id AS key, CAST(id AS string) AS value FROM range(1, 101)
""".stripMargin)

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.

Thanks for fixing it to loading the data only once!
But loading the data via JDBC statement was part of the testing coverage, to test executing those updates via JDBC. Instead of removing loadTestData, could you override createTestTables with creating and loading the data via JDBC statements?
While doing that, could you use statement.executeUpdate instead of statement.execute?

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.

hmm, I think we already have SQL tests for DDL/DML commands? If not we should add them.

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.

There are some tests that run a "CREATE TABLE" from within the test to set up some local data, but I believe not an extensive coverage.

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.

shall we create a test file to include all the temp views here and test reading/dropping?

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.

or we want to test executeUpdate explicitly?

sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
…a and test tables among multiple test cases
### What changes were proposed in this pull request?
This PR is related to apache#28060.
`ThriftServerQueryTestSuite` spend 17 minutes time to test.
I checked the code and found `ThriftServerQueryTestSuite` load test data repeatedly.
I've listed all the test cases order by time with desc in the `hive-thriftserver` module below.
Class | Spend time ↑ | Failure | Skip | Pass | Total test case
-- | -- | -- | -- | -- | --
ThriftServerQueryTestSuite | 17 minutes | 0 | 15 | 140 | 155
CliSuite | 8 minutes 24 seconds | 0 | 0 | 24 | 24
SparkThriftServerProtocolVersionsSuite | 59 seconds | 0 | 0 | 210 | 210
HiveThriftBinaryServerSuite | 36 seconds | 0 | 1 | 21 | 22
SparkMetadataOperationSuite | 19 seconds | 0 | 0 | 7 | 7
HiveCliSessionStateSuite | 16 seconds | 0 | 0 | 2 | 2
SparkSQLEnvSuite | 16 seconds | 0 | 0 | 1 | 1
HiveThriftHttpServerSuite | 15 seconds | 0 | 0 | 3 | 3
SingleSessionSuite | 14 seconds | 0 | 0 | 3 | 3
JdbcConnectionUriSuite | 2.1 seconds | 0 | 0 | 1 | 1
ThriftServerWithSparkContextSuite | 1.4 seconds | 0 | 0 | 1 | 1
SparkExecuteStatementOperationSuite | 63 millseconds | 0 | 0 | 2 | 2
UISeleniumSuite | -1 millseconds | 0 | 1 | 0 | 1
I checked the code of `ThriftServerQueryTestSuite` and found `ThriftServerQueryTestSuite` load test data repeatedly.
This PR will improve the performance of `ThriftServerQueryTestSuite`.
Because apache#28060 provides `createTestTables`(https://github.com/apache/spark/blob/e42a3945acd614a26c7941a9eed161b500fb4520/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L574) and `removeTestTables`(https://github.com/apache/spark/blob/e42a3945acd614a26c7941a9eed161b500fb4520/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L666), this PR will still uses them.
The total time run `ThriftServerQueryTestSuite` before and after this PR show below.
Before
No | Time
-- | --
1 | 18 minutes, 8 seconds
2 | 22 minutes, 44 seconds
3 | 17 minutes, 48 seconds
4 | 18 minutes, 30 seconds
After
No | Time
-- | --
1 | 16 minutes, 11 seconds
2 | 17 minutes, 19 seconds
3 | 18 minutes, 15 seconds
4 | 17 minutes, 27 seconds
### Why are the changes needed?
Improve the performance of `ThriftServerQueryTestSuite`.
### Does this PR introduce any user-facing change?
'No'.
### How was this patch tested?
Jenkins test
Closesapache#28180 from beliefer/avoid-load-thrift-test-data-repeatedly.
Authored-by: beliefer <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@beliefer
beliefer deleted the avoid-load-thrift-test-data-repeatedly branch April 23, 2024 07:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@beliefer@SparkQA@cloud-fan@maropu@dongjoon-hyun@juliuszsompolski