Skip to content

Commit

Permalink
[SPARK-29930][SQL][FOLLOW-UP] Allow only default value to be set for …
Browse files Browse the repository at this point in the history
…removed SQL configs

### What changes were proposed in this pull request?
In the PR, I propose to throw `AnalysisException` when a removed SQL config is set to non-default value. The following SQL configs removed by #26559 are marked as removed:
1. `spark.sql.fromJsonForceNullableSchema`
2. `spark.sql.legacy.compareDateTimestampInTimestamp`
3. `spark.sql.legacy.allowCreatingManagedTableUsingNonemptyLocation`

### Why are the changes needed?
To improve user experience with Spark SQL by notifying of removed SQL configs used by users.

### Does this PR introduce any user-facing change?
Yes, before the `set` command was silently ignored:
```sql
spark-sql> set spark.sql.fromJsonForceNullableSchema=false;
spark.sql.fromJsonForceNullableSchema	false
```
after the exception should be raised:
```sql
spark-sql> set spark.sql.fromJsonForceNullableSchema=false;
Error in query: The SQL config 'spark.sql.fromJsonForceNullableSchema' was removed in the version 3.0.0. It was removed to prevent errors like SPARK-23173 for non-default value.;
```

### How was this patch tested?
Added new tests into `SQLConfSuite` for both cases when removed SQL configs are set to default and non-default values.

Closes #27057 from MaxGekk/remove-sql-configs-followup.

Authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
  • Loading branch information
MaxGekk authored and HyukjinKwon committed Jan 3, 2020
1 parent 10cae04 commit a469976
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,36 @@ object SQLConf {
}
}

/**
* Holds information about keys that have been removed.
*
* @param key The removed config key.
* @param version Version of Spark where key was removed.
* @param defaultValue The default config value. It can be used to notice
* users that they set non-default value to an already removed config.
* @param comment Additional info regarding to the removed config.
*/
case class RemovedConfig(key: String, version: String, defaultValue: String, comment: String)

/**
* The map contains info about removed SQL configs. Keys are SQL config names,
* map values contain extra information like the version in which the config was removed,
* config's default value and a comment.
*/
val removedSQLConfigs: Map[String, RemovedConfig] = {
val configs = Seq(
RemovedConfig("spark.sql.fromJsonForceNullableSchema", "3.0.0", "true",
"It was removed to prevent errors like SPARK-23173 for non-default value."),
RemovedConfig(
"spark.sql.legacy.allowCreatingManagedTableUsingNonemptyLocation", "3.0.0", "false",
"It was removed to prevent loosing of users data for non-default value."),
RemovedConfig("spark.sql.legacy.compareDateTimestampInTimestamp", "3.0.0", "true",
"It was removed to prevent errors like SPARK-23549 for non-default value.")
)

Map(configs.map { cfg => cfg.key -> cfg } : _*)
}

val ANALYZER_MAX_ITERATIONS = buildConf("spark.sql.analyzer.maxIterations")
.internal()
.doc("The max number of iterations the analyzer runs.")
Expand Down
12 changes: 12 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package org.apache.spark.sql
import org.apache.spark.annotation.Stable
import org.apache.spark.internal.config.{ConfigEntry, OptionalConfigEntry}
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.RemovedConfig

/**
* Runtime configuration interface for Spark. To access this, use `SparkSession.conf`.
Expand All @@ -38,6 +39,7 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
*/
def set(key: String, value: String): Unit = {
requireNonStaticConf(key)
requireDefaultValueOfRemovedConf(key, value)
sqlConf.setConfString(key, value)
}

Expand Down Expand Up @@ -156,4 +158,14 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
throw new AnalysisException(s"Cannot modify the value of a Spark config: $key")
}
}

private def requireDefaultValueOfRemovedConf(key: String, value: String): Unit = {
SQLConf.removedSQLConfigs.get(key).foreach {
case RemovedConfig(configName, version, defaultValue, comment) =>
if (value != defaultValue) {
throw new AnalysisException(
s"The SQL config '$configName' was removed in the version $version. $comment")
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import org.apache.spark.sql.test.{SharedSparkSession, TestSQLContext}
import org.apache.spark.util.Utils

class SQLConfSuite extends QueryTest with SharedSparkSession {
import testImplicits._

private val testKey = "test.key.0"
private val testVal = "test.val.0"
Expand Down Expand Up @@ -320,4 +319,15 @@ class SQLConfSuite extends QueryTest with SharedSparkSession {
assert(e2.getMessage.contains("spark.sql.shuffle.partitions"))
}

test("set removed config to non-default value") {
val config = "spark.sql.fromJsonForceNullableSchema"
val defaultValue = true

spark.conf.set(config, defaultValue)

val e = intercept[AnalysisException] {
spark.conf.set(config, !defaultValue)
}
assert(e.getMessage.contains(config))
}
}

0 comments on commit a469976

Please sign in to comment.