[SPARK-26409][SQL][TESTS] SQLConf should be serializable in test sessions #23352

gengliangwang · 2018-12-19T17:09:22Z

What changes were proposed in this pull request?

SQLConf is supposed to be serializable. However, currently it is not serializable in WithTestConf. WithTestConf uses the method overrideConfs in closure, while the classes which implements it (TestHiveSessionStateBuilder and TestSQLSessionStateBuilder) are not serializable.

This PR is to use a local variable to fix it.

How was this patch tested?

Add unit test.

gengliangwang · 2018-12-19T17:13:29Z

@cloud-fan @HyukjinKwon @dongjoon-hyun

dongjoon-hyun · 2018-12-19T20:31:08Z

sql/core/src/test/scala/org/apache/spark/sql/SerializationSuite.scala

@@ -27,4 +27,9 @@ class SerializationSuite extends SparkFunSuite with SharedSQLContext {
    val spark = SparkSession.builder.getOrCreate()
    new JavaSerializer(new SparkConf()).newInstance().serialize(spark.sqlContext)
  }
+
+  test("[SPARK-26409] SQLConf should be serializable") {


[SPARK-26409] -> SPARK-26409?

It is following the other test case in the test suite. I can change it to either way. What do you think?

Yea, it should be okay as is. Honestly I want to to reformat all of them tho, it's obviously separate topic to discuss across the codebase.

dongjoon-hyun · 2018-12-19T20:31:48Z

sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala

    val conf = parentState.map(_.conf.clone()).getOrElse {
      new SQLConf {
        clear()
        override def clear(): Unit = {
          super.clear()
          // Make sure we start with the default test configs even after clear
-          overrideConfs.foreach { case (key, value) => setConfString(key, value) }
+          overrideConfigurations.foreach { case (key, value) => setConfString(key, value) }


Technically, this is touching src/main instead of src/test. But, no problem because it's WithTestConf. :)

maybe we should move this trait to src/test?

@cloud-fan agree.

I tried but failed.
Warning from IDE: Interface org.apache.spark.sql.internal.WithTestConf, referenced in file TestHive.scala, will not be accessible from module spark-hive

dongjoon-hyun · 2018-12-19T20:36:26Z

Hi, @gengliangwang . WithTestConf is very old one. Could you check the old Spark versions and update the Affected Versions?

SparkQA · 2018-12-19T21:19:49Z

Test build #100298 has finished for PR 23352 at commit 4b293e4.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2018-12-19T21:34:45Z

I'm not sure if PySpark test failure is flaky or not. Let's do test again.

FAIL: test_training_and_prediction (pyspark.mllib.tests.test_streaming_algorithms.StreamingLogisticRegressionWithSGDTests)
Test that the model improves on toy data with no. of batches
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/mllib/tests/test_streaming_algorithms.py", line 367, in test_training_and_prediction
    self._eventually(condition, timeout=60.0)
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/mllib/tests/test_streaming_algorithms.py", line 69, in _eventually
    lastValue = condition()
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/mllib/tests/test_streaming_algorithms.py", line 362, in condition
    self.assertGreater(errors[1] - errors[-1], 0.3)
AssertionError: -0.070000000000000062 not greater than 0.3

----------------------------------------------------------------------
Ran 13 tests in 184.315s

FAILED (failures=1, skipped=1)

dongjoon-hyun · 2018-12-19T21:34:52Z

Retest this please.

SparkQA · 2018-12-20T01:14:47Z

Test build #100319 has finished for PR 23352 at commit 4b293e4.

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

gengliangwang · 2018-12-20T03:40:31Z

@dongjoon-hyun I checked and the issue exists since 2.2.0.
ea36116

gatorsmile

LGTM

Thanks! Merged to master.

dongjoon-hyun · 2018-12-20T18:41:09Z

@gatorsmile . I'm wondering if we can backport this to the old branches at least 2.4?

gatorsmile · 2018-12-20T18:48:19Z

Since this only impacts the tests, I am fine either way.

dongjoon-hyun · 2018-12-20T20:03:44Z

Thanks, @gatorsmile . I'll backport to branch-2.4.

…ions ## What changes were proposed in this pull request? `SQLConf` is supposed to be serializable. However, currently it is not serializable in `WithTestConf`. `WithTestConf` uses the method `overrideConfs` in closure, while the classes which implements it (`TestHiveSessionStateBuilder` and `TestSQLSessionStateBuilder`) are not serializable. This PR is to use a local variable to fix it. ## How was this patch tested? Add unit test. Closes #23352 from gengliangwang/serializableSQLConf. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit 6692bac) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ions ## What changes were proposed in this pull request? `SQLConf` is supposed to be serializable. However, currently it is not serializable in `WithTestConf`. `WithTestConf` uses the method `overrideConfs` in closure, while the classes which implements it (`TestHiveSessionStateBuilder` and `TestSQLSessionStateBuilder`) are not serializable. This PR is to use a local variable to fix it. ## How was this patch tested? Add unit test. Closes apache#23352 from gengliangwang/serializableSQLConf. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…ions ## What changes were proposed in this pull request? `SQLConf` is supposed to be serializable. However, currently it is not serializable in `WithTestConf`. `WithTestConf` uses the method `overrideConfs` in closure, while the classes which implements it (`TestHiveSessionStateBuilder` and `TestSQLSessionStateBuilder`) are not serializable. This PR is to use a local variable to fix it. ## How was this patch tested? Add unit test. Closes apache#23352 from gengliangwang/serializableSQLConf. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit 6692bac) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

fix

4b293e4

dongjoon-hyun reviewed Dec 19, 2018

View reviewed changes

cloud-fan approved these changes Dec 20, 2018

View reviewed changes

HyukjinKwon approved these changes Dec 20, 2018

View reviewed changes

gatorsmile reviewed Dec 20, 2018

View reviewed changes

asfgit closed this in 6692bac Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26409][SQL][TESTS] SQLConf should be serializable in test sessions #23352

[SPARK-26409][SQL][TESTS] SQLConf should be serializable in test sessions #23352

gengliangwang commented Dec 19, 2018 •

edited

gengliangwang commented Dec 19, 2018

dongjoon-hyun Dec 19, 2018

gengliangwang Dec 20, 2018

HyukjinKwon Dec 20, 2018

dongjoon-hyun Dec 19, 2018 •

edited

cloud-fan Dec 20, 2018

gengliangwang Dec 20, 2018

gengliangwang Dec 20, 2018

dongjoon-hyun commented Dec 19, 2018

SparkQA commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

SparkQA commented Dec 20, 2018

gengliangwang commented Dec 20, 2018

gatorsmile left a comment

dongjoon-hyun commented Dec 20, 2018

gatorsmile commented Dec 20, 2018

dongjoon-hyun commented Dec 20, 2018

[SPARK-26409][SQL][TESTS] SQLConf should be serializable in test sessions #23352

[SPARK-26409][SQL][TESTS] SQLConf should be serializable in test sessions #23352

Conversation

gengliangwang commented Dec 19, 2018 • edited

What changes were proposed in this pull request?

How was this patch tested?

gengliangwang commented Dec 19, 2018

dongjoon-hyun Dec 19, 2018

Choose a reason for hiding this comment

gengliangwang Dec 20, 2018

Choose a reason for hiding this comment

HyukjinKwon Dec 20, 2018

Choose a reason for hiding this comment

dongjoon-hyun Dec 19, 2018 • edited

Choose a reason for hiding this comment

cloud-fan Dec 20, 2018

Choose a reason for hiding this comment

gengliangwang Dec 20, 2018

Choose a reason for hiding this comment

gengliangwang Dec 20, 2018

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 19, 2018

SparkQA commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

SparkQA commented Dec 20, 2018

gengliangwang commented Dec 20, 2018

gatorsmile left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 20, 2018

gatorsmile commented Dec 20, 2018

dongjoon-hyun commented Dec 20, 2018

gengliangwang commented Dec 19, 2018 •

edited

dongjoon-hyun Dec 19, 2018 •

edited