[SPARK-44034][TESTS] Add a new test group for sql module #41638

LuciferYang · 2023-06-17T07:44:17Z

What changes were proposed in this pull request?

The purpose of this pr is to add a new test tag SlowSQLTest to the sql module, and identified some Suites with test cases more than 3 seconds, and apply it to GA testing task to reduce the testing pressure of the sql others group.

For branches-3.3 and branches-3.4, a tag that will not appear in the sql module was assigned to the new test group to avoid java.lang.ClassNotFoundException and make this group build only without running any tests.

Why are the changes needed?

For a long time, the sql module UTs has only two groups: slow and others. The test cases in group slow are fixed, while the number of test cases in group others continues to increase, which has had a certain impact on the testing duration and stability of group others.

So this PR proposes to add a new testing group to share the testing pressure of sql others group, which has made the testing time of the three groups more average, and hope it can improve the stability of the GA task.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Should monitor GA

LuciferYang · 2023-06-17T07:47:31Z

friendly ping @HyukjinKwon @dongjoon-hyun @zhengruifeng

I close the old one because the commit record was not clean

dongjoon-hyun · 2023-06-18T04:13:01Z

sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala

@@ -220,7 +220,8 @@ class QueryExecutionSuite extends SharedSparkSession {
    assertNoTag(tag5, df.queryExecution.sparkPlan)
  }

-  test("Logging plan changes for execution") {
+  // TODO(SPARK-44074): re-enable this test after SPARK-44074 resolved


This failure is interesting.

Yes, interesting.

I found change SQLConf.PLAN_CHANGE_LOG_LEVEL.key from INFO to WARN should be ok.

But I haven't found the root cause yet

dongjoon-hyun · 2023-06-18T04:13:45Z

...rc/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableRenamePartitionSuite.scala

 import org.apache.spark.sql.Row
 import org.apache.spark.sql.execution.command

 /**
 * The class contains tests for the `ALTER TABLE .. RENAME PARTITION` command
 * to check V2 table catalogs.
 */
-@Ignore


Shall we do this separately? Maybe, we can do this first if this was due to some old issues.

This is because #41533 (comment) is also to avoid similar issues. I think it should not be the root cause, so we can remove ignore. I agree to remove ignore in an independent pr. let me revert this one first

test remove @ignore in #41647

dongjoon-hyun

+1, LGTM Thank you for this suggestion. This is a good new start to stabilize and re-balance everything in SQL module from now. I'll merge this, @LuciferYang . We can start from this baseline.

sql - extended tests (1h 44m 2s)
sql - slow tests (1h 18m 38s)
sql - other tests (1h 31m 30s)

dongjoon-hyun · 2023-06-18T21:07:23Z

For a record, although master branch becomes stabler in these days, we still have unstable Daily runs like the following. This PR helps us narrow-down the issues.

HyukjinKwon · 2023-06-18T23:48:55Z

👍

### What changes were proposed in this pull request? The purpose of this pr is to add a new test tag `SlowSQLTest` to the sql module, and identified some Suites with test cases more than 3 seconds, and apply it to GA testing task to reduce the testing pressure of the `sql others` group. For branches-3.3 and branches-3.4, a tag that will not appear in the sql module was assigned to the new test group to avoid `java.lang.ClassNotFoundException` and make this group build only without running any tests. ### Why are the changes needed? For a long time, the sql module UTs has only two groups: `slow` and `others`. The test cases in group `slow` are fixed, while the number of test cases in group `others` continues to increase, which has had a certain impact on the testing duration and stability of group `others`. So this PR proposes to add a new testing group to share the testing pressure of `sql others` group, which has made the testing time of the three groups more average, and hope it can improve the stability of the GA task. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Should monitor GA Closes apache#41638 from LuciferYang/SPARK-44034-2. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…rkFunSuite#withLogAppender` and re-enable UT `Logging plan changes for execution` ### What changes were proposed in this pull request? The main change of this pr is to add a call of `SparkFunSuite#wupdateLoggers` after restore loglevel when 'level' of `withLogAppender` function is not `None`, and under the premise of this change, the UT `Logging plan changes for execution` disabled in #41638 can be re-enabled. ### Why are the changes needed? - Fix bug of `SparkFunSuite#withLogAppender` when 'level' is not None - Re-enable UT `Logging plan changes for execution` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Manual test ``` build/sbt "sql/testOnly org.apache.spark.sql.JoinHintSuite org.apache.spark.sql.execution.QueryExecutionSuite" ``` **Before** ``` [info] - Logging plan changes for execution *** FAILED *** (36 milliseconds) [info] testAppender.loggingEvents.exists(((x$10: org.apache.logging.log4j.core.LogEvent) => x$10.getMessage().getFormattedMessage().contains(expectedMsg))) was false (QueryExecutionSuite.scala:232) [info] org.scalatest.exceptions.TestFailedException: [info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472) [info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471) [info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231) [info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295) [info] at org.apache.spark.sql.execution.QueryExecutionSuite.$anonfun$new$34(QueryExecutionSuite.scala:232) [info] at scala.collection.immutable.List.foreach(List.scala:431) [info] at org.apache.spark.sql.execution.QueryExecutionSuite.$anonfun$new$31(QueryExecutionSuite.scala:231) [info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) [info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) [info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) [info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) [info] at org.scalatest.Transformer.apply(Transformer.scala:22) ... ``` The failure reason is `withLogAppender(hintAppender, level = Some(Level.WARN))` used in `JoinHintSuite`, but `SparkFunSuite#wupdateLoggers` doesn't have the correct restore Loglevel. The test was successful before SPARK-44034 due to there was `AdaptiveQueryExecSuite` between `JoinHintSuite` and `QueryExecutionSuite`, and `AdaptiveQueryExecSuite` called `withLogAppender(hintAppender, level = Some(Level.DEBUG))`, but `AdaptiveQueryExecSuite` move to `slow sql` test group after SPARK-44034 **After** ``` [info] Run completed in 7 seconds, 485 milliseconds. [info] Total number of tests run: 32 [info] Suites: completed 2, aborted 0 [info] Tests: succeeded 32, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` Closes #41663 from LuciferYang/SPARK-44074. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

sunchao · 2023-09-26T17:32:40Z

@LuciferYang @dongjoon-hyun do you think it's a good idea to backport this to branch-3.4 (and potentially 3.3) to reduce the CI time?

dongjoon-hyun · 2023-09-26T17:37:00Z

+1 for @sunchao 's suggestion.

HyukjinKwon · 2023-09-27T00:39:16Z

Yeah, I don't mind backporting this.

LuciferYang · 2023-09-27T00:42:02Z

fine to me , I will submit a new PR for this.

sunchao · 2023-09-27T01:47:11Z

Great, thanks @LuciferYang !

The purpose of this pr is to add a new test tag `SlowSQLTest` to the sql module, and identified some Suites with test cases more than 3 seconds, and apply it to GA testing task to reduce the testing pressure of the `sql others` group. For branches-3.3 and branches-3.4, a tag that will not appear in the sql module was assigned to the new test group to avoid `java.lang.ClassNotFoundException` and make this group build only without running any tests. For a long time, the sql module UTs has only two groups: `slow` and `others`. The test cases in group `slow` are fixed, while the number of test cases in group `others` continues to increase, which has had a certain impact on the testing duration and stability of group `others`. So this PR proposes to add a new testing group to share the testing pressure of `sql others` group, which has made the testing time of the three groups more average, and hope it can improve the stability of the GA task. No Should monitor GA Closes apache#41638 from LuciferYang/SPARK-44034-2. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…rkFunSuite#withLogAppender` and re-enable UT `Logging plan changes for execution` ### What changes were proposed in this pull request? The main change of this pr is to add a call of `SparkFunSuite#wupdateLoggers` after restore loglevel when 'level' of `withLogAppender` function is not `None`, and under the premise of this change, the UT `Logging plan changes for execution` disabled in apache#41638 can be re-enabled. ### Why are the changes needed? - Fix bug of `SparkFunSuite#withLogAppender` when 'level' is not None - Re-enable UT `Logging plan changes for execution` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Manual test ``` build/sbt "sql/testOnly org.apache.spark.sql.JoinHintSuite org.apache.spark.sql.execution.QueryExecutionSuite" ``` **Before** ``` [info] - Logging plan changes for execution *** FAILED *** (36 milliseconds) [info] testAppender.loggingEvents.exists(((x$10: org.apache.logging.log4j.core.LogEvent) => x$10.getMessage().getFormattedMessage().contains(expectedMsg))) was false (QueryExecutionSuite.scala:232) [info] org.scalatest.exceptions.TestFailedException: [info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472) [info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471) [info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231) [info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295) [info] at org.apache.spark.sql.execution.QueryExecutionSuite.$anonfun$new$34(QueryExecutionSuite.scala:232) [info] at scala.collection.immutable.List.foreach(List.scala:431) [info] at org.apache.spark.sql.execution.QueryExecutionSuite.$anonfun$new$31(QueryExecutionSuite.scala:231) [info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) [info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) [info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) [info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) [info] at org.scalatest.Transformer.apply(Transformer.scala:22) ... ``` The failure reason is `withLogAppender(hintAppender, level = Some(Level.WARN))` used in `JoinHintSuite`, but `SparkFunSuite#wupdateLoggers` doesn't have the correct restore Loglevel. The test was successful before SPARK-44034 due to there was `AdaptiveQueryExecSuite` between `JoinHintSuite` and `QueryExecutionSuite`, and `AdaptiveQueryExecSuite` called `withLogAppender(hintAppender, level = Some(Level.DEBUG))`, but `AdaptiveQueryExecSuite` move to `slow sql` test group after SPARK-44034 **After** ``` [info] Run completed in 7 seconds, 485 milliseconds. [info] Total number of tests run: 32 [info] Suites: completed 2, aborted 0 [info] Tests: succeeded 32, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` Closes apache#41663 from LuciferYang/SPARK-44074. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

add new test group

dff0451

github-actions bot added INFRA SQL STRUCTURED STREAMING WEB UI labels Jun 17, 2023

LuciferYang mentioned this pull request Jun 17, 2023

[SPARK-44034][INFRA] Add a new test group for sql module #41552

Closed

Merge branch 'upmaster' into SPARK-44034-2

2e597ab

dongjoon-hyun reviewed Jun 18, 2023

View reviewed changes

revert change of AlterTableRenamePartitionSuite

dda6fbf

dongjoon-hyun changed the title ~~[SPARK-44034][INFRA] Add a new test group for sql module~~ [SPARK-44034][TESTS] Add a new test group for sql module Jun 18, 2023

dongjoon-hyun approved these changes Jun 18, 2023

View reviewed changes

dongjoon-hyun closed this in 1379680 Jun 18, 2023

LuciferYang mentioned this pull request Jun 20, 2023

[SPARK-44074][CORE][SQL][TESTS] Fix loglevel restore behavior of SparkFunSuite#withLogAppender and re-enable UT Logging plan changes for execution #41663

Closed

LuciferYang mentioned this pull request Sep 28, 2023

[SPARK-44074][CORE][SQL][TESTS][3.4] Fix loglevel restore behavior of SparkFunSuite#withLogAppender and re-enable UT Logging plan changes for execution #43157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44034][TESTS] Add a new test group for sql module #41638

[SPARK-44034][TESTS] Add a new test group for sql module #41638

LuciferYang commented Jun 17, 2023

LuciferYang commented Jun 17, 2023

dongjoon-hyun Jun 18, 2023

LuciferYang Jun 18, 2023

dongjoon-hyun Jun 18, 2023

LuciferYang Jun 18, 2023

LuciferYang Jun 18, 2023

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun commented Jun 18, 2023 •

edited

Loading

HyukjinKwon commented Jun 18, 2023

sunchao commented Sep 26, 2023

dongjoon-hyun commented Sep 26, 2023

HyukjinKwon commented Sep 27, 2023

LuciferYang commented Sep 27, 2023

sunchao commented Sep 27, 2023

[SPARK-44034][TESTS] Add a new test group for sql module #41638

[SPARK-44034][TESTS] Add a new test group for sql module #41638

Conversation

LuciferYang commented Jun 17, 2023

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?

LuciferYang commented Jun 17, 2023

dongjoon-hyun Jun 18, 2023

Choose a reason for hiding this comment

LuciferYang Jun 18, 2023

Choose a reason for hiding this comment

dongjoon-hyun Jun 18, 2023

Choose a reason for hiding this comment

LuciferYang Jun 18, 2023

Choose a reason for hiding this comment

LuciferYang Jun 18, 2023

Choose a reason for hiding this comment

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun commented Jun 18, 2023 • edited Loading

HyukjinKwon commented Jun 18, 2023

sunchao commented Sep 26, 2023

dongjoon-hyun commented Sep 26, 2023

HyukjinKwon commented Sep 27, 2023

LuciferYang commented Sep 27, 2023

sunchao commented Sep 27, 2023

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun commented Jun 18, 2023 •

edited

Loading