-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
e8de7bb
33d831e
e19b1e4
3f1841c
0320857
37ebae4
336a331
1f703aa
6c64431
3e42660
4e7b6bb
96b2280
dd49ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ import org.apache.spark.util.Utils | |
|
|
||
| object SQLConf { | ||
|
|
||
| private val sqlConfEntries = java.util.Collections.synchronizedMap( | ||
| private[sql] val sqlConfEntries = java.util.Collections.synchronizedMap( | ||
| new java.util.HashMap[String, ConfigEntry[_]]()) | ||
|
|
||
| val staticConfKeys: java.util.Set[String] = | ||
|
|
@@ -1610,6 +1610,14 @@ object SQLConf { | |
| """ "... N more fields" placeholder.""") | ||
| .intConf | ||
| .createWithDefault(25) | ||
|
|
||
| val SET_COMMAND_REJECTS_SPARK_CONFS = | ||
| buildConf("spark.sql.execution.setCommandRejectsSparkConfs") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we use the legacy prefix? |
||
| .internal() | ||
| .doc("If it is set to true, SET command will fail when the key is registered as " + | ||
| "an SparkConf entry.") | ||
| .booleanConf | ||
| .createWithDefault(true) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2030,6 +2038,8 @@ class SQLConf extends Serializable with Logging { | |
|
|
||
| def maxToStringFields: Int = getConf(SQLConf.MAX_TO_STRING_FIELDS) | ||
|
|
||
| def setCommandRejectsSparkConfs: Boolean = getConf(SQLConf.SET_COMMAND_REJECTS_SPARK_CONFS) | ||
|
|
||
| /** ********************** SQLConf functionality methods ************ */ | ||
|
|
||
| /** Set Spark SQL configuration properties. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,5 +153,9 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) { | |
| if (SQLConf.staticConfKeys.contains(key)) { | ||
| throw new AnalysisException(s"Cannot modify the value of a static config: $key") | ||
| } | ||
| if (sqlConf.setCommandRejectsSparkConfs && | ||
| ConfigEntry.findEntry(key) != null && !SQLConf.sqlConfEntries.containsKey(key)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, thinking of it, isn't Removing that would also want by other settings that don't have constants defined.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry for the delay.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should only reject configs that are registered as SparkConf. Thinking about configs that are neither a SparkConf or SQLConf, we shouldn't reject it. |
||
| throw new AnalysisException(s"Cannot modify the value of a Spark config: $key") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,4 +68,13 @@ class RuntimeConfigSuite extends SparkFunSuite { | |
| assert(!conf.isModifiable("")) | ||
| assert(!conf.isModifiable("invalid config parameter")) | ||
| } | ||
|
|
||
| test("reject SparkConf entries") { | ||
| val conf = newConf() | ||
|
|
||
| val ex = intercept[AnalysisException] { | ||
| conf.set("spark.task.cpus", 4) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use |
||
| } | ||
| assert(ex.getMessage.contains("Spark config")) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2715,4 +2715,11 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils { | |
| } | ||
| } | ||
| } | ||
|
|
||
| test("set command rejects SparkConf entries") { | ||
| val ex = intercept[AnalysisException] { | ||
| sql("SET spark.task.cpus = 4") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| } | ||
| assert(ex.getMessage.contains("Spark config")) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I looked at that code again, the static config part already threw an error before, so probably can be removed here.