[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031

ueshin · 2018-11-14T09:13:40Z

What changes were proposed in this pull request?

Currently 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. We should track SparkConf entries and make the command reject for such entries.

How was this patch tested?

Added a test and existing tests.

SparkQA · 2018-11-14T09:20:42Z

Test build #98817 has finished for PR 23031 at commit e19b1e4.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-14T12:48:22Z

Test build #98818 has finished for PR 23031 at commit 3f1841c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-11-14T13:19:10Z

Jenkins, retest this please.

SparkQA · 2018-11-14T16:40:29Z

Test build #98827 has finished for PR 23031 at commit 3f1841c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-11-14T22:12:47Z

All entries are already captured in the ConfigEntry object, so are all these changes really needed?

You could warn if anybody tries to update something that's in there, but not in SQLConf.sqlConfEntries. Also, be warned that this doesn't cover all config options, since lots of old options do not have a constant.

ueshin · 2018-11-15T05:30:30Z

@vanzin Thanks for letting me know it and that's really good to know.
I'll update this to use it.

SparkQA · 2018-11-15T08:05:01Z

Test build #98858 has finished for PR 23031 at commit 37ebae4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-11-15T08:07:55Z

Jenkins, retest this please.

SparkQA · 2018-11-15T11:56:00Z

Test build #98862 has finished for PR 23031 at commit 37ebae4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin

LGTM.

vanzin · 2018-11-15T19:56:11Z

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")


vanzin · 2018-11-15T19:58:04Z

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`.


...fails if a non-SQL or static SQL config key is used.

SparkQA · 2018-11-16T06:11:00Z

Test build #98896 has finished for PR 23031 at commit 336a331.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-11-16T19:01:12Z

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)) {


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.

I'm sorry for the delay.
As per the comment #22887 (comment), I'd leave it as is for now.

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.

vanzin · 2018-11-16T19:01:55Z

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`.


Now that I looked at that code again, the static config part already threw an error before, so probably can be removed here.

cloud-fan · 2018-11-28T13:06:32Z

sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala

+    val conf = newConf()
+
+    val ex = intercept[AnalysisException] {
+      conf.set("spark.task.cpus", 4)


can we use config.CPUS_PER_TASK instead of hardcoding it?

cloud-fan · 2018-11-28T13:06:43Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

+
+  test("set command rejects SparkConf entries") {
+    val ex = intercept[AnalysisException] {
+      sql("SET spark.task.cpus = 4")


SparkQA · 2018-11-28T15:59:19Z

Test build #99373 has finished for PR 23031 at commit 1f703aa.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-28T16:49:20Z

Test build #99374 has finished for PR 23031 at commit 4e7b6bb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-28T20:05:10Z

Test build #99388 has finished for PR 23031 at commit 96b2280.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ArrayBasedMapBuilder(keyType: DataType, valueType: DataType) extends Serializable

ueshin · 2018-11-29T02:15:51Z

Jenkins, retest this please.

SparkQA · 2018-11-29T06:03:24Z

Test build #99422 has finished for PR 23031 at commit 96b2280.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ArrayBasedMapBuilder(keyType: DataType, valueType: DataType) extends Serializable

cloud-fan · 2018-11-29T06:18:10Z

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")


shall we use the legacy prefix?

SparkQA · 2018-11-29T08:05:02Z

Test build #99434 has finished for PR 23031 at commit dd49ea3.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-11-29T09:32:00Z

Jenkins, retest this please.

SparkQA · 2018-11-29T12:03:48Z

Test build #99444 has finished for PR 23031 at commit dd49ea3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-11-29T13:58:34Z

Jenkins, retest this please.

SparkQA · 2018-11-29T15:45:32Z

Test build #99455 has finished for PR 23031 at commit dd49ea3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-11-29T15:49:31Z

Jenkins, retest this please.

SparkQA · 2018-11-29T19:33:37Z

Test build #99463 has finished for PR 23031 at commit dd49ea3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-11-30T03:56:46Z

thanks, merging to master!

…t such entries. ## What changes were proposed in this pull request? Currently 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. We should track `SparkConf` entries and make the command reject for such entries. ## How was this patch tested? Added a test and existing tests. Closes apache#23031 from ueshin/issues/SPARK-26060/set_command. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

## What changes were proposed in this pull request? This is a follow-up of apache#23031 to rename the config name to `spark.sql.legacy.setCommandRejectsSparkCoreConfs`. ## How was this patch tested? Existing tests. Closes apache#23245 from ueshin/issues/SPARK-26060/rename_config. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

ueshin added 3 commits November 14, 2018 16:25

Register SparkConfig entries.

e8de7bb

Make SET command reject SparkConf entries.

33d831e

Update the migration guide.

e19b1e4

Fix.

3f1841c

ueshin added 2 commits November 15, 2018 13:51

Revert "Register SparkConfig entries." and related changes.

0320857

Use ConfigEntry.findEntry().

37ebae4

ueshin changed the title ~~[SPARK-26060][CORE][SQL] Track SparkConf entries and make SET command reject such entries.~~ [SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. Nov 15, 2018

vanzin reviewed Nov 15, 2018

View reviewed changes

Address comments.

336a331

vanzin reviewed Nov 16, 2018

View reviewed changes

vanzin mentioned this pull request Nov 28, 2018

[SPARK-25880][CORE] user set's hadoop conf should not overwrite by sparkcontext's conf #22887

Closed

Merge branch 'master' into issues/SPARK-26060/set_command

1f703aa

cloud-fan reviewed Nov 28, 2018

View reviewed changes

ueshin added 3 commits November 28, 2018 22:08

Update the migration guide.

6c64431

Use config.CPUS_PER_TASK instead of hardcoding it.

3e42660

Fix.

4e7b6bb

Merge branch 'master' into issues/SPARK-26060/set_command

96b2280

cloud-fan reviewed Nov 29, 2018

View reviewed changes

Add the legacy prefix.

dd49ea3

asfgit closed this in 8edb64c Nov 30, 2018

ueshin mentioned this pull request Dec 6, 2018

[SPARK-26060][SQL][FOLLOW-UP] Rename the config name. #23245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031

[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031

ueshin commented Nov 14, 2018

SparkQA commented Nov 14, 2018

SparkQA commented Nov 14, 2018

ueshin commented Nov 14, 2018

SparkQA commented Nov 14, 2018

vanzin commented Nov 14, 2018

ueshin commented Nov 15, 2018

SparkQA commented Nov 15, 2018

ueshin commented Nov 15, 2018

SparkQA commented Nov 15, 2018

vanzin left a comment

vanzin Nov 15, 2018

vanzin Nov 15, 2018

SparkQA commented Nov 16, 2018

vanzin Nov 16, 2018

ueshin Nov 28, 2018

cloud-fan Nov 28, 2018 •

edited

Loading

vanzin Nov 16, 2018

cloud-fan Nov 28, 2018

cloud-fan Nov 28, 2018

SparkQA commented Nov 28, 2018

SparkQA commented Nov 28, 2018

SparkQA commented Nov 28, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

cloud-fan Nov 29, 2018

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

cloud-fan commented Nov 30, 2018

		@@ -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`.

[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031

[SPARK-26060][SQL] Track SparkConf entries and make SET command reject such entries. #23031

Conversation

ueshin commented Nov 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 14, 2018

SparkQA commented Nov 14, 2018

ueshin commented Nov 14, 2018

SparkQA commented Nov 14, 2018

vanzin commented Nov 14, 2018

ueshin commented Nov 15, 2018

SparkQA commented Nov 15, 2018

ueshin commented Nov 15, 2018

SparkQA commented Nov 15, 2018

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 28, 2018

SparkQA commented Nov 28, 2018

SparkQA commented Nov 28, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

Choose a reason for hiding this comment

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

ueshin commented Nov 29, 2018

SparkQA commented Nov 29, 2018

cloud-fan commented Nov 30, 2018

cloud-fan Nov 28, 2018 •

edited

Loading