From 0741c5d6678992de9ee3827bb3c6428466f9217c Mon Sep 17 00:00:00 2001 From: Tejas Patil Date: Wed, 20 Jan 2016 20:06:49 +0530 Subject: [PATCH 1/3] [SPARK-12926][SQL] SQLContext to disallow users passing non-sql configs --- .../main/scala/org/apache/spark/sql/SQLConf.scala | 14 ++++++++++++-- .../scala/org/apache/spark/sql/SQLConfSuite.scala | 11 +++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala index 2d664d3ee691b..af808fc801e2a 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala @@ -19,6 +19,8 @@ package org.apache.spark.sql import java.util.Properties +import org.apache.spark.SparkException + import scala.collection.immutable import scala.collection.JavaConverters._ @@ -618,7 +620,7 @@ private[sql] class SQLConf extends Serializable with CatalystConf with ParserCon // Only verify configs in the SQLConf object entry.valueConverter(value) } - settings.put(key, value) + setConfWithCheck(key, value) } /** Set the given Spark SQL configuration property. */ @@ -626,7 +628,7 @@ private[sql] class SQLConf extends Serializable with CatalystConf with ParserCon require(entry != null, "entry cannot be null") require(value != null, s"value cannot be null for key: ${entry.key}") require(sqlConfEntries.get(entry.key) == entry, s"$entry is not registered") - settings.put(entry.key, entry.stringConverter(value)) + setConfWithCheck(entry.key, entry.stringConverter(value)) } /** Return the value of Spark SQL configuration property for the given key. */ @@ -689,6 +691,14 @@ private[sql] class SQLConf extends Serializable with CatalystConf with ParserCon }.toSeq } + private def setConfWithCheck(key: String, value: String): Unit = { + if (key.startsWith("spark.") && !key.startsWith("spark.sql.")) { + throw new SparkException( + s"Attempt to set non-Spark SQL config in SQLConf: key = $key, value = $value") + } + settings.put(key, value) + } + private[spark] def unsetConf(key: String): Unit = { settings.remove(key) } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala index cf0701eca29ea..9dc72a66c9eed 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala @@ -17,6 +17,7 @@ package org.apache.spark.sql +import org.apache.spark.SparkException import org.apache.spark.sql.test.{SharedSQLContext, TestSQLContext} class SQLConfSuite extends QueryTest with SharedSQLContext { @@ -93,6 +94,16 @@ class SQLConfSuite extends QueryTest with SharedSQLContext { assert(e.getMessage === s"${SQLConf.CASE_SENSITIVE.key} should be boolean, but was 10") } + test("passing Spark config other than Spark SQL ones") { + sqlContext.conf.clear() + val key = "spark.some.config" + val value = "foo" + val e = intercept[SparkException] { + sql(s"set $key=$value") + } + assert(e.getMessage === s"Attempt to set non-Spark SQL config in SQLConf: key = $key, value = $value") + } + test("Test SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE's method") { sqlContext.conf.clear() From 7f1e317b5120461c001f947eb43cecc80776f87c Mon Sep 17 00:00:00 2001 From: Tejas Patil Date: Thu, 21 Jan 2016 08:29:27 +0530 Subject: [PATCH 2/3] Changed from exception to WARN statement. Testing: ``` scala> sqlContext.sql("SET spark.shuffle.memoryFraction=0.4") 16/01/21 08:26:47 WARN SQLConf: Attempt to set non-Spark SQL config in SQLConf: key = spark.shuffle.memoryFraction, value = 0.4 res2: org.apache.spark.sql.DataFrame = [key: string, value: string] scala> sqlContext.getConf("spark.shuffle.memoryFraction") res3: String = 0.4 ``` --- .../src/main/scala/org/apache/spark/sql/SQLConf.scala | 7 +++---- .../scala/org/apache/spark/sql/SQLConfSuite.scala | 11 ----------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala index af808fc801e2a..66439d061ed40 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala @@ -19,7 +19,7 @@ package org.apache.spark.sql import java.util.Properties -import org.apache.spark.SparkException +import org.apache.spark.Logging import scala.collection.immutable import scala.collection.JavaConverters._ @@ -513,7 +513,7 @@ private[spark] object SQLConf { * * SQLConf is thread-safe (internally synchronized, so safe to be used in multiple threads). */ -private[sql] class SQLConf extends Serializable with CatalystConf with ParserConf { +private[sql] class SQLConf extends Serializable with CatalystConf with ParserConf with Logging { import SQLConf._ /** Only low degree of contention is expected for conf, thus NOT using ConcurrentHashMap. */ @@ -693,8 +693,7 @@ private[sql] class SQLConf extends Serializable with CatalystConf with ParserCon private def setConfWithCheck(key: String, value: String): Unit = { if (key.startsWith("spark.") && !key.startsWith("spark.sql.")) { - throw new SparkException( - s"Attempt to set non-Spark SQL config in SQLConf: key = $key, value = $value") + logWarning(s"Attempt to set non-Spark SQL config in SQLConf: key = $key, value = $value") } settings.put(key, value) } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala index 9dc72a66c9eed..cf0701eca29ea 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLConfSuite.scala @@ -17,7 +17,6 @@ package org.apache.spark.sql -import org.apache.spark.SparkException import org.apache.spark.sql.test.{SharedSQLContext, TestSQLContext} class SQLConfSuite extends QueryTest with SharedSQLContext { @@ -94,16 +93,6 @@ class SQLConfSuite extends QueryTest with SharedSQLContext { assert(e.getMessage === s"${SQLConf.CASE_SENSITIVE.key} should be boolean, but was 10") } - test("passing Spark config other than Spark SQL ones") { - sqlContext.conf.clear() - val key = "spark.some.config" - val value = "foo" - val e = intercept[SparkException] { - sql(s"set $key=$value") - } - assert(e.getMessage === s"Attempt to set non-Spark SQL config in SQLConf: key = $key, value = $value") - } - test("Test SHUFFLE_TARGET_POSTSHUFFLE_INPUT_SIZE's method") { sqlContext.conf.clear() From f982d5449fc52ef9b844761f92306fb7d238b542 Mon Sep 17 00:00:00 2001 From: Tejas Patil Date: Tue, 26 Jan 2016 13:00:09 +0530 Subject: [PATCH 3/3] Fixed scala checkstyle. Verified by running: `sbt sql/compile:scalastyle` --- sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala index 66439d061ed40..f9eb8e4be7b2d 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala @@ -19,13 +19,12 @@ package org.apache.spark.sql import java.util.Properties -import org.apache.spark.Logging - import scala.collection.immutable import scala.collection.JavaConverters._ import org.apache.parquet.hadoop.ParquetOutputCommitter +import org.apache.spark.Logging import org.apache.spark.sql.catalyst.CatalystConf import org.apache.spark.sql.catalyst.parser.ParserConf import org.apache.spark.util.Utils