[SPARK-34660][TESTS][3.1] Don't use ParVector with `withExistingConf` which is not thread-safe #31775

dongjoon-hyun · 2021-03-08T07:06:07Z

What changes were proposed in this pull request?

This PR aims to remove ParVector usage with withExistingConf in CastSuite in order to fix UT failures.

This was added by SPARK-33498 .
master branch doesn't use ParVector.

- new ParVector(ALL_TIMEZONES.toVector).foreach { zid =>
+ ALL_TIMEZONES.foreach { zid =>

Why are the changes needed?

withExistingConf is not thread-safe like the following. We should not use ParVector on it.

def withExistingConf[T](conf: SQLConf)(f: => T): T = {
  val old = existingConf.get()
  existingConf.set(conf)
  try {
    f
  } finally {
    if (old != null) {
      existingConf.set(old)
    } else {
      existingConf.remove()
    }
  }
}

Does this PR introduce any user-facing change?

BEFORE

$ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested"
...
[info] *** 4 TESTS FAILED ***
[error] Failed: Total 8, Failed 4, Errors 0, Passed 4
[error] Failed tests:
[error] 	org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite

AFTER

$ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested"
...
[info] All tests passed.

How was this patch tested?

Pass the CIs on branch-3.1. Currently, it's broken.

…h is not thread-safe

dongjoon-hyun · 2021-03-08T07:13:57Z

cc @leanken , @cloud-fan , @rednaxelafx , @maropu , @kiszk , @viirya

kiszk · 2021-03-08T07:18:00Z

Great analysis!

dongjoon-hyun · 2021-03-08T07:21:30Z

Thank you, @kiszk !

dongjoon-hyun · 2021-03-08T07:41:22Z

This is a test-only PR and I tested this manually. Merged to branch-3.1 to recover the branch.

… which is not thread-safe ### What changes were proposed in this pull request? This PR aims to remove `ParVector` usage with `withExistingConf` in `CastSuite` in order to fix UT failures. - This was added by SPARK-33498 . - `master` branch doesn't use `ParVector`. ```scala - new ParVector(ALL_TIMEZONES.toVector).foreach { zid => + ALL_TIMEZONES.foreach { zid => ``` ### Why are the changes needed? `withExistingConf` is not thread-safe like the following. We should not use `ParVector` on it. ```scala def withExistingConf[T](conf: SQLConf)(f: => T): T = { val old = existingConf.get() existingConf.set(conf) try { f } finally { if (old != null) { existingConf.set(old) } else { existingConf.remove() } } } ``` ### Does this PR introduce _any_ user-facing change? **BEFORE** ``` $ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested" ... [info] *** 4 TESTS FAILED *** [error] Failed: Total 8, Failed 4, Errors 0, Passed 4 [error] Failed tests: [error] org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite ``` **AFTER** ``` $ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested" ... [info] All tests passed. ``` ### How was this patch tested? Pass the CIs on `branch-3.1`. Currently, it's broken. Closes #31775 from dongjoon-hyun/SPARK-34660. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

maropu · 2021-03-08T07:44:58Z

late lgtm.

cloud-fan · 2021-03-08T07:48:33Z

late LGTM

maropu · 2021-03-08T07:49:10Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

    val activeConf = conf
-    new ParVector(ALL_TIMEZONES.toVector).foreach { zid =>
+    ALL_TIMEZONES.foreach { zid =>
      def checkCastWithParseError(str: String): Unit = {


nit: It looks like we don't need to wrap SQLConf.withExistingConf now.

viirya · 2021-03-08T08:08:53Z

late lgtm

SparkQA · 2021-03-08T11:17:37Z

Test build #135857 has finished for PR 31775 at commit 6bfb870.

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

dongjoon-hyun · 2021-03-08T16:06:31Z

Thank you, @maropu , @cloud-fan , @viirya .

BTW, @maropu . Could you make a follow-up if needed, please?

nit: It looks like we don't need to wrap SQLConf.withExistingConf now.

maropu · 2021-03-09T00:52:58Z

Yea, I'll do it later.

…n CastSuite ### What changes were proposed in this pull request? This PR intends to remove unnecessary `SQLConf.withExistingConf` in `CastSuite`; since we've remove `ParVector ` in #31775, we no longer need to copy SQL configs into each thread env. ### Why are the changes needed? Clean up the code. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Run the existing tests. Closes #31785 from maropu/UpdateCastSuite. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…onf in CastSuite ### What changes were proposed in this pull request? This PR intends to remove unnecessary `SQLConf.withExistingConf` in `CastSuite`; since we've remove `ParVector ` in #31775, we no longer need to copy SQL configs into each thread env. This PR is a backport of #31785. ### Why are the changes needed? Clean up the code. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Run the existing tests. Closes #31813 from maropu/SPARK-33498-BRANCH3.1. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… which is not thread-safe This PR aims to remove `ParVector` usage with `withExistingConf` in `CastSuite` in order to fix UT failures. - This was added by SPARK-33498 . - `master` branch doesn't use `ParVector`. ```scala - new ParVector(ALL_TIMEZONES.toVector).foreach { zid => + ALL_TIMEZONES.foreach { zid => ``` `withExistingConf` is not thread-safe like the following. We should not use `ParVector` on it. ```scala def withExistingConf[T](conf: SQLConf)(f: => T): T = { val old = existingConf.get() existingConf.set(conf) try { f } finally { if (old != null) { existingConf.set(old) } else { existingConf.remove() } } } ``` **BEFORE** ``` $ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested" ... [info] *** 4 TESTS FAILED *** [error] Failed: Total 8, Failed 4, Errors 0, Passed 4 [error] Failed tests: [error] org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite ``` **AFTER** ``` $ build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z parse -z nested" ... [info] All tests passed. ``` Pass the CIs on `branch-3.1`. Currently, it's broken. Closes apache#31775 from dongjoon-hyun/SPARK-34660. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…onf in CastSuite ### What changes were proposed in this pull request? This PR intends to remove unnecessary `SQLConf.withExistingConf` in `CastSuite`; since we've remove `ParVector ` in apache#31775, we no longer need to copy SQL configs into each thread env. This PR is a backport of apache#31785. ### Why are the changes needed? Clean up the code. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Run the existing tests. Closes apache#31813 from maropu/SPARK-33498-BRANCH3.1. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

[SPARK-34660][TESTS] Don't use ParVector with withExistingConf whic…

6bfb870

…h is not thread-safe

github-actions bot added the SQL label Mar 8, 2021

dongjoon-hyun changed the title ~~[SPARK-34660][TESTS] Don't use ParVector with withExistingConf which is not thread-safe~~ [SPARK-34660][TESTS][3.1] Don't use ParVector with withExistingConf which is not thread-safe Mar 8, 2021

kiszk approved these changes Mar 8, 2021

View reviewed changes

dongjoon-hyun mentioned this pull request Mar 8, 2021

[SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS][3.1] Add PlanTest. testFallback and use it #31764

Closed

1 task

dongjoon-hyun closed this Mar 8, 2021

dongjoon-hyun deleted the SPARK-34660 branch March 8, 2021 07:42

maropu reviewed Mar 8, 2021

View reviewed changes

maropu mentioned this pull request Mar 9, 2021

[SPARK-33498][SQL][TESTS][FOLLOWUP] Remove SQLConf.withExistingConf in CastSuite #31785

Closed

dongjoon-hyun mentioned this pull request Mar 10, 2021

[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766

Closed

maropu mentioned this pull request Mar 12, 2021

[SPARK-33498][SQL][TESTS][3.1][FOLLOWUP] Remove SQLConf.withExistingConf in CastSuite #31813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-34660][TESTS][3.1] Don't use ParVector with `withExistingConf` which is not thread-safe #31775

[SPARK-34660][TESTS][3.1] Don't use ParVector with `withExistingConf` which is not thread-safe #31775

Uh oh!

dongjoon-hyun commented Mar 8, 2021 •

edited

Loading

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

kiszk commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

maropu commented Mar 8, 2021

Uh oh!

cloud-fan commented Mar 8, 2021

Uh oh!

maropu Mar 8, 2021

Uh oh!

viirya commented Mar 8, 2021

Uh oh!

SparkQA commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021 •

edited

Loading

Uh oh!

maropu commented Mar 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-34660][TESTS][3.1] Don't use ParVector with withExistingConf which is not thread-safe #31775

[SPARK-34660][TESTS][3.1] Don't use ParVector with withExistingConf which is not thread-safe #31775

Uh oh!

Conversation

dongjoon-hyun commented Mar 8, 2021 • 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!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

kiszk commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021

Uh oh!

maropu commented Mar 8, 2021

Uh oh!

cloud-fan commented Mar 8, 2021

Uh oh!

maropu Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 8, 2021

Uh oh!

SparkQA commented Mar 8, 2021

Uh oh!

dongjoon-hyun commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Mar 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-34660][TESTS][3.1] Don't use ParVector with `withExistingConf` which is not thread-safe #31775

[SPARK-34660][TESTS][3.1] Don't use ParVector with `withExistingConf` which is not thread-safe #31775

dongjoon-hyun commented Mar 8, 2021 •

edited

Loading

dongjoon-hyun commented Mar 8, 2021 •

edited

Loading