From f8983d1b7c00cb7f61d3fccd6baefe1dc9345232 Mon Sep 17 00:00:00 2001
From: Zongheng Yang <zongheng.y@gmail.com>
Date: Thu, 5 Jun 2014 21:19:05 -0700
Subject: [PATCH] Minor changes per review comments.

---
 .../main/scala/org/apache/spark/sql/SQLConf.scala   | 13 ++++++-------
 .../org/apache/spark/sql/execution/commands.scala   |  5 +++++
 .../org/apache/spark/sql/hive/HiveContext.scala     |  3 +--
 .../scala/org/apache/spark/sql/hive/HiveQl.scala    |  2 ++
 4 files changed, 14 insertions(+), 9 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 78ba270b02c27..4411a086e7d72 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
@@ -22,7 +22,10 @@ import java.util.Properties
 import scala.collection.mutable
 
 /**
- * SQLConf holds potentially query-dependent, mutable config parameters and hints.
+ * SQLConf holds mutable config parameters and hints.  These can be set and
+ * queried either by passing SET commands into Spark SQL's DSL
+ * functions (sql(), hql(), etc.), or by programmatically using setters and
+ * getters of this class.
  */
 class SQLConf {
 
@@ -39,12 +42,8 @@ class SQLConf {
   }
 
   def set(key: String, value: String): SQLConf = {
-    if (key == null) {
-      throw new NullPointerException("null key")
-    }
-    if (value == null) {
-      throw new NullPointerException("null value")
-    }
+    require(key != null, "key cannot be null")
+    require(value != null, s"value cannot be null for ${key}")
     settings(key) = value
     this
   }
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala
index 52bfd9678ae2e..84ae8d23a0f24 100644
--- a/sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala
+++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala
@@ -29,13 +29,17 @@ import org.apache.spark.sql.catalyst.expressions.{GenericRow, Attribute}
 case class SetCommandPhysical(key: Option[String], value: Option[String])
                              (@transient context: SQLContext) extends LeafNode {
    def execute(): RDD[Row] = (key, value) match {
+     // Set value for key k; the action itself would
+     // have been performed in QueryExecution eagerly.
      case (Some(k), Some(v)) => context.emptyResult
+     // Query the value bound to key k.
      case (Some(k), None) =>
        val resultString = context.sqlConf.getOption(k) match {
          case Some(v) => s"$k=$v"
          case None => s"$k is undefined"
        }
        context.sparkContext.parallelize(Seq(new GenericRow(Array[Any](resultString))), 1)
+     // Query all key-value pairs that are set in the SQLConf of the context.
      case (None, None) =>
        val pairs = context.sqlConf.getAll
        val rows = pairs.map { case (k, v) =>
@@ -43,6 +47,7 @@ case class SetCommandPhysical(key: Option[String], value: Option[String])
        }.toSeq
        // Assume config parameters can fit into one split (machine) ;)
        context.sparkContext.parallelize(rows, 1)
+     // The only other case is invalid semantics and is impossible.
      case _ => context.emptyResult
    }
 
diff --git a/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveContext.scala b/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveContext.scala
index 2b09cea12965b..a12f5e6faefb1 100644
--- a/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveContext.scala
+++ b/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveContext.scala
@@ -133,8 +133,7 @@ class HiveContext(sc: SparkContext) extends SQLContext(sc) {
   @transient override lazy val sqlConf: SQLConf = new SQLConf(hiveconf.getAllProperties) {
     override def set(key: String, value: String): SQLConf = {
       runSqlHive(s"SET $key=$value")
-      settings(key) = value
-      this
+      super.set(key, value)
     }
   }
   @transient protected[hive] lazy val sessionState = new SessionState(hiveconf)
diff --git a/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala b/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala
index caa7515919004..83393b3d867f2 100644
--- a/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala
+++ b/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala
@@ -208,6 +208,8 @@ private[hive] object HiveQl {
   def parseSql(sql: String): LogicalPlan = {
     try {
       if (sql.trim.toLowerCase.startsWith("set")) {
+        // Split in two parts since we treat the part before the first "="
+        // as key, and the part after as value, which may contain other "=" signs.
         sql.trim.drop(3).split("=", 2).map(_.trim) match {
           case Array("") => // "set"
             SetCommand(None, None)