[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/23031 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r237364466
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1610,6 +1610,14 @@ object SQLConf {
""" "... N more fields" placeholder.""")
.intConf
.createWithDefault(25)
+
+ val SET_COMMAND_REJECTS_SPARK_CONFS =
+buildConf("spark.sql.execution.setCommandRejectsSparkConfs")
--- End diff --
shall we use the legacy prefix?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r237075228
--- Diff:
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
---
@@ -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")
--- End diff --
ditto
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r237075170
--- Diff:
sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala ---
@@ -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)
--- End diff --
can we use `config.CPUS_PER_TASK` instead of hardcoding it?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r237074727
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
---
@@ -154,5 +154,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)) {
--- End diff --
we should only reject configs that are registered as SparkConf. Thinking
about configs that are either a SparkConf or SQLConf, we shouldn't reject it.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r237071928
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
---
@@ -154,5 +154,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)) {
--- End diff --
I'm sorry for the delay.
As per the comment
https://github.com/apache/spark/pull/22887#issuecomment-442425557, I'd leave it
as is for now.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r234314168
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
---
@@ -154,5 +154,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)) {
--- End diff --
Actually, thinking of it, isn't `ConfigEntry.findEntry(key) != null`
redundant with the other check?
Removing that would also want by other settings that don't have constants
defined.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/23031#discussion_r234314374 --- Diff: docs/sql-migration-guide-upgrade.md --- @@ -17,6 +17,8 @@ displayTitle: Spark SQL Upgrading Guide - The `ADD JAR` command previously returned a result set with the single value 0. It now returns an empty result set. + - In Spark version 2.4 and earlier, the `SET` command works without any warnings even if the specified key is for `SparkConf` entries and it has no effect because the command does not update `SparkConf`, but the behavior might confuse users. Since 3.0, the command fails if a non-SQL or static SQL config key is used. You can disable such a check by setting `spark.sql.execution.setCommandRejectsSparkConfs` to `false`. --- End diff -- Now that I looked at that code again, the static config part already threw an error before, so probably can be removed here. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/23031#discussion_r233987571 --- Diff: docs/sql-migration-guide-upgrade.md --- @@ -17,6 +17,8 @@ displayTitle: Spark SQL Upgrading Guide - The `ADD JAR` command previously returned a result set with the single value 0. It now returns an empty result set. + - In Spark version 2.4 and earlier, the `SET` command works without any warnings even if the specified key is for `SparkConf` entries and it has no effect because the command does not update `SparkConf`, but the behavior might confuse users. Since 3.0, the command fails if the key is for `SparkConf` as same as for static sql config keys. You can disable such a check by setting `spark.sql.execution.setCommandRejectsSparkConfs` to `false`. --- End diff -- ...fails if a non-SQL or static SQL config key is used. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23031: [SPARK-26060][SQL] Track SparkConf entries and ma...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/23031#discussion_r233986989
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
---
@@ -154,5 +154,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)) {
+ throw new AnalysisException(s"Cannot modify the value of a spark
config: $key")
--- End diff --
Spark
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
