[SPARK-20946][SQL] simplify the config setting logic in SparkSession.getOrCreate #18172

cloud-fan · 2017-06-01T05:19:23Z

What changes were proposed in this pull request?

The current conf setting logic is a little complex and has duplication, this PR simplifies it.

How was this patch tested?

existing tests.

cloud-fan · 2017-06-01T05:20:06Z

mllib/src/test/scala/org/apache/spark/ml/tree/impl/TreeTests.scala

@@ -43,7 +43,6 @@ private[ml] object TreeTests extends SparkFunSuite {
      categoricalFeatures: Map[Int, Int],
      numClasses: Int): DataFrame = {
    val spark = SparkSession.builder()
-      .master("local[2]")


here we build SparkSession with an existing spark context, so setting the master is useless.

Would be great if we can check whether there are similar changes can be made.

cloud-fan · 2017-06-01T05:25:53Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

-          val sc = SparkContext.getOrCreate(sparkConf)
-          // maybe this is an existing SparkContext, update its SparkConf which maybe used
-          // by SparkSession
-          options.foreach { case (k, v) => sc.conf.set(k, v) }


previously we set the given options to the newly created SparkConf, then create SparkContext, then set the options to sc.conf again, in case the SparkContext.getOrCreate returns an existing one. We can just create SparkConf and use it to create SparkContext, and then set options to sc.conf, so that we only need to set the conf once.

cloud-fan · 2017-06-01T05:28:58Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

@@ -935,7 +929,6 @@ object SparkSession {
        }

        session = new SparkSession(sparkContext, None, None, extensions)
-        options.foreach { case (k, v) => session.sessionState.conf.setConfString(k, v) }


When building SessionState, we will merge spark conf to sql conf, which means we don't need to set options to session.sessionState.conf here, if the spark conf already contains options.

cloud-fan · 2017-06-01T05:31:52Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

@@ -899,21 +899,15 @@ object SparkSession {

        // No active nor global default session. Create a new one.
        val sparkContext = userSuppliedContext.getOrElse {
+          val sparkConf = new SparkConf()
+          SparkContext.getOrCreate(sparkConf)


in order to remove https://github.com/apache/spark/pull/18172/files#diff-d91c284798f1c98bf03a31855e26d71cL938 , here I change the behavior to also setting options to sparkContext.conf even if the sparkContext is supplied by users. This change is safe as SparkSession.Builder.sparkContext is private, and I checked all the callers to confirm that it's ok to do so.

Empty SparkConf lacking necessary setting such as spark.master causes failure.

SparkQA · 2017-06-01T05:44:49Z

Test build #77628 has finished for PR 18172 at commit 077795d.

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

cloud-fan · 2017-06-01T15:25:29Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

@@ -753,6 +753,8 @@ object SparkSession {

    private[this] val options = new scala.collection.mutable.HashMap[String, String]

+    private[this] var master: Option[String] = None


master and appName are only useful when creating a new SparkContext, so here I separate them from options and only use it when creating new SparkContext

cloud-fan · 2017-06-01T15:26:33Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

+          SparkContext.getOrCreate(sparkConf)
+        }
+
+        options.foreach { case (k, v) => sparkContext.conf.set(k, v) }


in order to remove https://github.com/apache/spark/pull/18172/files#diff-d91c284798f1c98bf03a31855e26d71cL938 , here I change the behavior to also setting options to sparkContext.conf even if the sparkContext is supplied by users. This change is safe as SparkSession.Builder.sparkContext is private, and I checked all the callers to confirm that it's ok to do so.

SparkQA · 2017-06-01T15:46:26Z

Test build #77643 has finished for PR 18172 at commit 3edec68.

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

cloud-fan · 2017-06-01T17:29:46Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

        }
+        options.foreach { case (k, v) => sparkContext.conf.set(k, v) }


in order to remove https://github.com/apache/spark/pull/18172/files#diff-d91c284798f1c98bf03a31855e26d71cL938 , here I change the behavior to also setting options to sparkContext.conf even if the sparkContext is supplied by users. This change is safe as SparkSession.Builder.sparkContext is private, and I checked all the callers to confirm that it's ok to do so.

SparkQA · 2017-06-01T19:01:09Z

Test build #77648 has finished for PR 18172 at commit b2a75fe.

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

cloud-fan · 2017-06-01T20:46:27Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

        }
+        options.foreach { case (k, v) => sparkContext.conf.set(k, v) }


in order to remove https://github.com/apache/spark/pull/18172/files#diff-d91c284798f1c98bf03a31855e26d71cL938 , here I change the behavior to also setting options to sparkContext.conf even if the sparkContext is supplied by users. This change is safe as SparkSession.Builder.sparkContext is private, and I checked all the callers to confirm that it's ok to do so.

Maybe we can add a comment to SparkSession.Builder.sparkContext that we will modify its conf?

yea good idea

SparkQA · 2017-06-01T22:56:45Z

Test build #77652 has finished for PR 18172 at commit ace2fbc.

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

cloud-fan · 2017-06-01T23:34:11Z

CC @liancheng

jiangxb1987

LGTM

jiangxb1987 · 2017-06-02T00:04:03Z

mllib/src/test/scala/org/apache/spark/ml/tree/impl/TreeTests.scala

@@ -43,7 +43,6 @@ private[ml] object TreeTests extends SparkFunSuite {
      categoricalFeatures: Map[Int, Int],
      numClasses: Int): DataFrame = {
    val spark = SparkSession.builder()
-      .master("local[2]")


Would be great if we can check whether there are similar changes can be made.

viirya · 2017-06-02T00:51:12Z

LGTM except for one comment regarding the behavior change.

SparkQA · 2017-06-02T05:53:24Z

Test build #77665 has finished for PR 18172 at commit a8fe8b0.

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

…getOrCreate ## What changes were proposed in this pull request? The current conf setting logic is a little complex and has duplication, this PR simplifies it. ## How was this patch tested? existing tests. Author: Wenchen Fan <wenchen@databricks.com> Closes #18172 from cloud-fan/session. (cherry picked from commit e11d90b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-06-02T17:05:38Z

thanks for the review, merging to master/2.2!

yhuai · 2017-06-02T22:34:38Z

Reverting this because it breaks repl tests.

cloud-fan commented Jun 1, 2017

View reviewed changes

cloud-fan force-pushed the session branch from 077795d to 3edec68 Compare June 1, 2017 15:23

cloud-fan commented Jun 1, 2017

View reviewed changes

cloud-fan force-pushed the session branch from 3edec68 to b2a75fe Compare June 1, 2017 17:28

cloud-fan commented Jun 1, 2017

View reviewed changes

simplify the config setting logic in SparkSession.getOrCreate

ace2fbc

cloud-fan force-pushed the session branch from b2a75fe to ace2fbc Compare June 1, 2017 20:45

cloud-fan commented Jun 1, 2017

View reviewed changes

jiangxb1987 approved these changes Jun 2, 2017

View reviewed changes

address comments

a8fe8b0

asfgit closed this in e11d90b Jun 2, 2017

This was referenced Jul 3, 2017

[SPARK-20256][SQL] SessionState should be created more lazily #18501

Closed

[SPARK-20946][SQL] Simplify the config setting logic in SparkSession.getOrCreate #18512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20946][SQL] simplify the config setting logic in SparkSession.getOrCreate #18172

[SPARK-20946][SQL] simplify the config setting logic in SparkSession.getOrCreate #18172

cloud-fan commented Jun 1, 2017

cloud-fan Jun 1, 2017

jiangxb1987 Jun 2, 2017

cloud-fan Jun 1, 2017

cloud-fan Jun 1, 2017

cloud-fan Jun 1, 2017 •

edited

Loading

viirya Jun 1, 2017

SparkQA commented Jun 1, 2017

cloud-fan Jun 1, 2017 •

edited

Loading

cloud-fan Jun 1, 2017

SparkQA commented Jun 1, 2017

cloud-fan Jun 1, 2017

SparkQA commented Jun 1, 2017

cloud-fan Jun 1, 2017

viirya Jun 2, 2017

cloud-fan Jun 2, 2017

SparkQA commented Jun 1, 2017

cloud-fan commented Jun 1, 2017

jiangxb1987 left a comment

jiangxb1987 Jun 2, 2017

viirya commented Jun 2, 2017

SparkQA commented Jun 2, 2017

cloud-fan commented Jun 2, 2017

yhuai commented Jun 2, 2017

		@@ -753,6 +753,8 @@ object SparkSession {

		private[this] val options = new scala.collection.mutable.HashMap[String, String]

		private[this] var master: Option[String] = None

		}
		options.foreach { case (k, v) => sparkContext.conf.set(k, v) }

[SPARK-20946][SQL] simplify the config setting logic in SparkSession.getOrCreate #18172

[SPARK-20946][SQL] simplify the config setting logic in SparkSession.getOrCreate #18172

Conversation

cloud-fan commented Jun 1, 2017

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 1, 2017

cloud-fan Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 1, 2017

Choose a reason for hiding this comment

SparkQA commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 1, 2017

cloud-fan commented Jun 1, 2017

jiangxb1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Jun 2, 2017

SparkQA commented Jun 2, 2017

cloud-fan commented Jun 2, 2017

yhuai commented Jun 2, 2017

cloud-fan Jun 1, 2017 •

edited

Loading

cloud-fan Jun 1, 2017 •

edited

Loading