[SPARK-31234][SQL] ResetCommand should reset config to sc.conf only #28003

yaooqinn · 2020-03-24T13:30:48Z

What changes were proposed in this pull request?

Currently, ResetCommand clear all configurations, including sql configs, static sql configs and spark context level configs.
for example:

spark-sql> set xyz=abc;
xyz abc
spark-sql> set;
spark.app.id local-1585055396930
spark.app.name SparkSQL::10.242.189.214
spark.driver.host 10.242.189.214
spark.driver.port 65094
spark.executor.id driver
spark.jars
spark.master local[*]
spark.sql.catalogImplementation hive
spark.sql.hive.version 1.2.1
spark.submit.deployMode client
xyz abc
spark-sql> reset;
spark-sql> set;
spark-sql> set spark.sql.hive.version;
spark.sql.hive.version 1.2.1
spark-sql> set spark.app.id;
spark.app.id <undefined>

In this PR, we restore spark confs to RuntimeConfig after it is cleared

Why are the changes needed?

reset command overkills configs which are static.

Does this PR introduce any user-facing change?

yes, the ResetCommand do not change static configs now

How was this patch tested?

add ut

SparkQA · 2020-03-24T16:53:33Z

Test build #120275 has finished for PR 28003 at commit 9de836e.

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

SparkQA · 2020-03-24T19:47:24Z

Test build #120278 has finished for PR 28003 at commit 3b6b3aa.

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

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

yaooqinn · 2020-03-25T05:21:24Z

also, cc @cloud-fan @gatorsmile thanks

yaooqinn · 2020-03-25T05:23:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

    unsetConf(entry.key)
  }

  def clear(): Unit = {


This API has been exposed to users since 2.1, and makes the SQLConf hard to be restored to its initial state after calling spark.sessionState.conf.clear

SQLConf is internnal.

scala> spark.sessionState.conf.clear scala> spark.sessionState.conf res1: org.apache.spark.sql.internal.SQLConf = org.apache.spark.sql.internal.SQLConf@54e24511

show we make it private[sql] ?

We don't. All classes under org.apache.spark.sql.internal are internal and users can still use. Spark marks things as internal by contract.

cloud-fan · 2020-03-25T06:15:01Z

docs/sql-ref-syntax-aux-conf-mgmt-reset.md


 ### Description
-Reset all the properties specific to the current session to their default values. After RESET command, executing SET command will output empty.
+Reset any properties specific to the current session which were set via the [SET](sql-ref-syntax-aux-conf-mgmt-set.html) command to their default values.


"any properties" -> "any runtime configurations"

cloud-fan · 2020-03-25T06:15:51Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

  }

  def clear(): Unit = {
+    val reserved = settings.asScala.filterKeys(staticConfKeys.contains).toMap


does staticConfKeys include the core configs?

it doesn't. If a user calls it, it will make the SparkConfs in SQLConf all gone

cloud-fan · 2020-03-25T06:16:42Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

 /**
- * This command is for resetting SQLConf to the default values. Command that runs
+ * This command is for resetting SQLConf to the default values. Any configurations that were set
+ * via [[SetCommand]] will get reset to default value Command that runs


a dot before Command that runs?

SparkQA · 2020-03-25T07:05:01Z

Test build #120309 has finished for PR 28003 at commit 435e411.

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

SparkQA · 2020-03-25T07:05:02Z

Test build #120318 has finished for PR 28003 at commit bcc6786.

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

yaooqinn · 2020-03-25T07:57:54Z

retest this please

cloud-fan · 2020-03-25T08:36:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

  def clear(): Unit = {
+    val reserved = settings.asScala.filterKeys(staticConfKeys.contains).toMap
    settings.clear()
+    settings.putAll(reserved.asJava)


is this change necessary? static SQL configs can only be set with SparkConf. We can clear everything here, and the RESET command will add back the configs in SparkConf

yes. it is not necessary.

cloud-fan · 2020-03-25T10:31:41Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

    spark.sessionState.conf.clear()
    // After clear, only overrideConfs used by unit test should be in the SQLConf.
-    assert(spark.conf.getAll === TestSQLContext.overrideConfs)
+    assert((spark.conf.getAll -- TestSQLContext.overrideConfs.keys).size < spark.conf.getAll.size)


is this still needed?

cloud-fan · 2020-03-25T10:31:54Z

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

      sql("SELECT * FROM lowerCaseData INTERSECT SELECT * FROM upperCaseData"), Nil)
  }

  test("SET commands semantics using sql()") {


do we still need to change?

SparkQA · 2020-03-25T11:36:52Z

Test build #120342 has finished for PR 28003 at commit 8363a14.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-03-25T11:43:58Z

docs/sql-ref-syntax-aux-conf-mgmt-reset.md

 ### Description
-Reset all the properties specific to the current session to their default values. After RESET command, executing SET command will output empty.
+Reset any runtime configurations specific to the current session which were set via the [SET](sql-ref-syntax-aux-conf-mgmt-set.html) command to their default values.



We need to update the comment below, too?

-- Reset "all the properties" specific to the current session to their default values. RESET;

good catch!

maropu · 2020-03-25T11:47:57Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

    }
  }

+  test("reset - static and spark conf") {


nit: the test name had better be self-descriptive.

SparkQA · 2020-03-25T12:23:36Z

Test build #120346 has finished for PR 28003 at commit 45c926e.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-03-25T12:30:25Z

retest this please

cloud-fan · 2020-03-25T13:13:49Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

+    val warehouseDir = conf.get("spark.sql.warehouse.dir")
+    // ensure the conf here is not default value, and will not be reset to default value later
+    assert(warehouseDir.get.contains(this.getClass.getCanonicalName))
+    sql(s"reset")


no need the s

and it's better to upper case the SQL keyword.

SparkQA · 2020-03-25T13:19:51Z

Test build #120338 has finished for PR 28003 at commit 69a4b82.

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

SparkQA · 2020-03-25T13:32:18Z

Test build #120325 has finished for PR 28003 at commit bcc6786.

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

SparkQA · 2020-03-25T18:08:07Z

Test build #120349 has finished for PR 28003 at commit 45c926e.

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

SparkQA · 2020-03-25T18:53:41Z

Test build #120357 has finished for PR 28003 at commit d374c57.

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

maropu · 2020-03-25T22:42:23Z

Does this PR introduce any user-facing change?
yes, the ResetCommand do not change static configs now

btw, any chance to silently change behaviour of existing apps by this fix? We don't need to keep the current behaviour by a legacy config? cc: @gatorsmile @dongjoon-hyun @HyukjinKwon

cloud-fan · 2020-03-26T05:49:21Z

I think this is clearly a bug. If users set a static SQL config in the config file, this should not be affected by SET or RESET.

cloud-fan · 2020-03-26T06:29:12Z

thanks, merging to master/3.0!

### What changes were proposed in this pull request? Currently, ResetCommand clear all configurations, including sql configs, static sql configs and spark context level configs. for example: ```sql spark-sql> set xyz=abc; xyz abc spark-sql> set; spark.app.id local-1585055396930 spark.app.name SparkSQL::10.242.189.214 spark.driver.host 10.242.189.214 spark.driver.port 65094 spark.executor.id driver spark.jars spark.master local[*] spark.sql.catalogImplementation hive spark.sql.hive.version 1.2.1 spark.submit.deployMode client xyz abc spark-sql> reset; spark-sql> set; spark-sql> set spark.sql.hive.version; spark.sql.hive.version 1.2.1 spark-sql> set spark.app.id; spark.app.id <undefined> ``` In this PR, we restore spark confs to RuntimeConfig after it is cleared ### Why are the changes needed? reset command overkills configs which are static. ### Does this PR introduce any user-facing change? yes, the ResetCommand do not change static configs now ### How was this patch tested? add ut Closes #28003 from yaooqinn/SPARK-31234. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 44bd36a) Signed-off-by: Wenchen Fan <[email protected]>

gatorsmile · 2020-04-01T02:32:11Z

@yaooqinn Could you backport this to 2.4? and add it to the migration guide since it could impact the query results?

yaooqinn · 2020-04-01T04:53:09Z

@yaooqinn Could you backport this to 2.4? and add it to the migration guide since it could impact the query results?

Hi @gatorsmile, why the migration guide should I put? In the backport or a followup to master?

### What changes were proposed in this pull request? Currently, ResetCommand clear all configurations, including sql configs, static sql configs and spark context level configs. for example: ```sql spark-sql> set xyz=abc; xyz abc spark-sql> set; spark.app.id local-1585055396930 spark.app.name SparkSQL::10.242.189.214 spark.driver.host 10.242.189.214 spark.driver.port 65094 spark.executor.id driver spark.jars spark.master local[*] spark.sql.catalogImplementation hive spark.sql.hive.version 1.2.1 spark.submit.deployMode client xyz abc spark-sql> reset; spark-sql> set; spark-sql> set spark.sql.hive.version; spark.sql.hive.version 1.2.1 spark-sql> set spark.app.id; spark.app.id <undefined> ``` In this PR, we restore spark confs to RuntimeConfig after it is cleared ### Why are the changes needed? reset command overkills configs which are static. ### Does this PR introduce any user-facing change? yes, the ResetCommand do not change static configs now ### How was this patch tested? add ut Closes apache#28003 from yaooqinn/SPARK-31234. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…figuration ### What changes were proposed in this pull request? This PR is to backport the fix of #28003, add a migration guide, update the PR description and add an end-to-end test case. Before this PR, the SQL `RESET` command will reset the values of static SQL configuration to the default and remove the cached values of Spark Context Configurations in the current session. This PR fixes the bugs. After this PR, the `RESET` command follows its definition and only updates the runtime SQL configuration values to the default. ### Why are the changes needed? When we introduced the feature of Static SQL Configuration, we did not update the implementation of SQL `RESET` command. The static SQL configuration should not be changed by any command at runtime. However, the `RESET` command resets the values to the default. We should fix them. ### Does this PR introduce any user-facing change? Before Spark 2.4.6, the `RESET` command resets both the runtime and static SQL configuration values to the default. It also removes the cached values of Spark Context Configurations in the current session, although these configuration values are for displaying/querying only. ### How was this patch tested? Added an end-to-end test and a unit test Closes #28262 from gatorsmile/spark-31234followup2.4. Authored-by: gatorsmile <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…QL Configuration ### What changes were proposed in this pull request? This PR is the follow-up PR of #28003 - add a migration guide - add an end-to-end test case. ### Why are the changes needed? The original PR made the major behavior change in the user-facing RESET command. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added a new end-to-end test Closes #28265 from gatorsmile/spark-31234followup. Authored-by: gatorsmile <[email protected]> Signed-off-by: gatorsmile <[email protected]>

…QL Configuration ### What changes were proposed in this pull request? This PR is the follow-up PR of #28003 - add a migration guide - add an end-to-end test case. ### Why are the changes needed? The original PR made the major behavior change in the user-facing RESET command. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added a new end-to-end test Closes #28265 from gatorsmile/spark-31234followup. Authored-by: gatorsmile <[email protected]> Signed-off-by: gatorsmile <[email protected]> (cherry picked from commit 6c792a7) Signed-off-by: gatorsmile <[email protected]>

yaooqinn added 2 commits March 24, 2020 21:26

[SPARK-31234][SQL] ResetCommand should reset config to sc.conf only

9de836e

nit

3b6b3aa

maropu reviewed Mar 24, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala Show resolved Hide resolved

fix tests

435e411

yaooqinn commented Mar 25, 2020

View reviewed changes

cloud-fan reviewed Mar 25, 2020

View reviewed changes

yaooqinn added 2 commits March 25, 2020 14:38

nnit

ef63604

nnit

bcc6786

cloud-fan reviewed Mar 25, 2020

View reviewed changes

address cmts

69a4b82

cloud-fan reviewed Mar 25, 2020

View reviewed changes

yaooqinn added 2 commits March 25, 2020 19:06

unnecessary

460c35b

unnecessary

8363a14

maropu reviewed Mar 25, 2020

View reviewed changes

address comments

45c926e

cloud-fan reviewed Mar 25, 2020

View reviewed changes

nit

d374c57

cloud-fan closed this in 44bd36a Mar 26, 2020

dongjoon-hyun added the SQL label Mar 27, 2020

This was referenced Apr 19, 2020

[SPARK-31234][SQL][2.4] ResetCommand should not affect static SQL Configuration #28262

Closed

[SPARK-31234][SQL][FOLLOW-UP] ResetCommand should not affect static SQL Configuration #28265

Closed

[SPARK-31234][SQL] ResetCommand should reset config to sc.conf only #28003

[SPARK-31234][SQL] ResetCommand should reset config to sc.conf only #28003

Uh oh!

Conversation

yaooqinn commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Mar 24, 2020

Uh oh!

SparkQA commented Mar 24, 2020

Uh oh!

Uh oh!

yaooqinn commented Mar 25, 2020

Uh oh!

yaooqinn Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

yaooqinn commented Mar 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

yaooqinn commented Mar 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

yaooqinn commented Mar 24, 2020 •

edited

Loading

yaooqinn Mar 25, 2020 •

edited

Loading

cloud-fan Mar 25, 2020 •

edited

Loading

yaooqinn Mar 25, 2020 •

edited

Loading