[SPARK-30416][SQL] Log a warning for deprecated SQL config in `set()` and `unset()` #27092

MaxGekk · 2020-01-03T16:29:15Z

What changes were proposed in this pull request?

Put all deprecated SQL configs the map SQLConf.deprecatedSQLConfigs with extra info about when configs were deprecated and additional comments that explain why a config was deprecated, what an user can use instead of it. Here is the list of already deprecated configs:
- spark.sql.hive.verifyPartitionPath
- spark.sql.execution.pandas.respectSessionTimeZone
- spark.sql.legacy.execution.pandas.groupedMap.assignColumnsByName
- spark.sql.parquet.int64AsTimestampMillis
- spark.sql.variable.substitute.depth
- spark.sql.execution.arrow.enabled
- spark.sql.execution.arrow.fallback.enabled
Output warning in set() and unset() about deprecated SQL configs

Why are the changes needed?

This should improve UX with Spark SQL and notify users about already deprecated SQL configs.

Does this PR introduce any user-facing change?

Yes, before:

spark-sql> set spark.sql.hive.verifyPartitionPath=true;
spark.sql.hive.verifyPartitionPath	true

After:

spark-sql> set spark.sql.hive.verifyPartitionPath=true;
20/01/03 21:28:17 WARN RuntimeConfig: The SQL config 'spark.sql.hive.verifyPartitionPath' has been deprecated in Spark v3.0.0 and may be removed in the future. This config is replaced by spark.files.ignoreMissingFiles.
spark.sql.hive.verifyPartitionPath	true

How was this patch tested?

Add new test which registers new log appender and catches all logging to check that set() and unset() log any warning.

SparkQA · 2020-01-03T20:29:36Z

Test build #116099 has finished for PR 27092 at commit 722ddaa.

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

MaxGekk · 2020-01-04T16:22:35Z

@HyukjinKwon Please, have a look at the PR.

MaxGekk · 2020-01-06T10:31:20Z

@cloud-fan @maropu Please, have a look at the PR.

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

SparkQA · 2020-01-06T16:04:50Z

Test build #116164 has finished for PR 27092 at commit f9a2dcd.

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

maropu · 2020-01-08T00:40:19Z

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

  }
+
+  test("log deprecation warnings") {
+    val logAppender = new AppenderSkeleton {


nit: The same class logAppender seems to be defined in some places below, so can we define a helper method for this test purpose somewhere (e.g., TestUtils)?

$grep -nr "extends AppenderSkeleton" . ./catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveHintsSuite.scala:36: class MockAppender extends AppenderSkeleton { ./catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGenerationSuite.scala:525: class MockAppender extends AppenderSkeleton { ./catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizerLoggingSuite.scala:42: class MockAppender extends AppenderSkeleton { ./core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala:1766: class TestAppender extends AppenderSkeleton { ./core/src/test/scala/org/apache/spark/sql/JoinHintSuite.scala:41: class MockAppender extends AppenderSkeleton {

it is slightly orthogonal to the PR but if you think it makes sense I will do that here.

Yea, I think its ok in follow-up.

maropu

This looks useful! LGTM. cc: @HyukjinKwon

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

MaxGekk · 2020-01-08T11:31:20Z

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

+   */
+  val deprecatedSQLConfigs: Map[String, DeprecatedConfig] = {
+    val configs = Seq(
+      DeprecatedConfig(VARIABLE_SUBSTITUTE_DEPTH.key, "2.1",


I haven't found where this config is used. We can remove it, I think.

MaxGekk · 2020-01-08T11:36:57Z

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

    .doc("When true, check all the partition paths under the table\'s root directory " +
         "when reading data stored in HDFS. This configuration will be deprecated in the future " +
-         "releases and replaced by spark.files.ignoreMissingFiles.")
+         s"releases and replaced by ${SPARK_IGNORE_MISSING_FILES.key}.")


@cloud-fan Regarding to your comment #19868 (comment), spark.sql.hive.verifyPartitionPath can be changed at runtime but spark.files.ignoreMissingFiles cannot be. Is it fair replacement?

We can just slightly reword that users can use spark.files.ignoreMissingFiles instead of saying it's a replacement if you're concerned. I don't believe this configuration is commonly used enough, and it should be fine.

If users find this is unreasonable, we can un-deprecate it later given the feedback.

SparkQA · 2020-01-08T13:40:21Z

Test build #116292 has finished for PR 27092 at commit e93f587.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class DeprecatedConfig(key: String, version: String, comment: String)

SparkQA · 2020-01-08T15:46:05Z

Test build #116299 has finished for PR 27092 at commit 496ca8c.

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

HyukjinKwon · 2020-01-09T03:54:26Z

@MaxGekk mind resolving conflicts?

…cated-sql-configs # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

SparkQA · 2020-01-09T08:05:01Z

Test build #116349 has finished for PR 27092 at commit faf88e8.

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

MaxGekk · 2020-01-09T08:28:15Z

jenkins, retest this, please

SparkQA · 2020-01-09T13:02:16Z

Test build #116371 has finished for PR 27092 at commit faf88e8.

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

HyukjinKwon · 2020-01-09T13:33:37Z

@viirya, mind asking it again to the r sysadmin? It starts to fail again:

* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]
Execution halted

viirya · 2020-01-09T16:58:23Z

@viirya, mind asking it again to the r sysadmin? It starts to fail again:
* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]
Execution halted

Sure, just asked CRAN admin for help. :)

viirya · 2020-01-09T20:57:57Z

@HyukjinKwon Although I don't get reply, looks like it was fixed because I saw successful build from other PR.

viirya · 2020-01-09T20:58:06Z

retest this please

SparkQA · 2020-01-10T01:24:44Z

Test build #116416 has finished for PR 27092 at commit faf88e8.

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

HyukjinKwon · 2020-01-10T01:32:02Z

Thanks, @viirya

Merged to master.

MaxGekk added 3 commits January 3, 2020 20:27

Add deprecatedSQLConfigs

a59c700

Implement logDeprecationWarning()

73a3790

Add a test

722ddaa

MaxGekk mentioned this pull request Jan 3, 2020

[SPARK-29930][SQL][FOLLOW-UP] Allow only default value to be set for removed SQL configs #27057

Closed

cloud-fan reviewed Jan 6, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

will be replaced -> is replaced

f9a2dcd

maropu reviewed Jan 8, 2020

View reviewed changes

maropu approved these changes Jan 8, 2020

View reviewed changes

HyukjinKwon reviewed Jan 8, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

MaxGekk added 2 commits January 8, 2020 12:29

Use config keys

e93f587

Add other deprecated configs

496ca8c

MaxGekk commented Jan 8, 2020

View reviewed changes

HyukjinKwon approved these changes Jan 9, 2020

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into group-depre…

faf88e8

…cated-sql-configs # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

HyukjinKwon closed this in 1ffa627 Jan 10, 2020

MaxGekk mentioned this pull request Jan 10, 2020

[SPARK-30482][SQL][CORE][TESTS] Add sub-class of AppenderSkeleton reusable in tests #27166

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

MaxGekk deleted the group-deprecated-sql-configs branch June 5, 2020 19:42

[SPARK-30416][SQL] Log a warning for deprecated SQL config in set() and unset() #27092

[SPARK-30416][SQL] Log a warning for deprecated SQL config in set() and unset() #27092

Uh oh!

Conversation

MaxGekk commented Jan 3, 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 Jan 3, 2020

Uh oh!

MaxGekk commented Jan 4, 2020

Uh oh!

MaxGekk commented Jan 6, 2020

Uh oh!

Uh oh!

SparkQA commented Jan 6, 2020

Uh oh!

maropu Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxGekk Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 8, 2020

Uh oh!

SparkQA commented Jan 8, 2020

Uh oh!

HyukjinKwon commented Jan 9, 2020

Uh oh!

SparkQA commented Jan 9, 2020

Uh oh!

MaxGekk commented Jan 9, 2020

Uh oh!

SparkQA commented Jan 9, 2020

Uh oh!

HyukjinKwon commented Jan 9, 2020

Uh oh!

viirya commented Jan 9, 2020

Uh oh!

viirya commented Jan 9, 2020

Uh oh!

viirya commented Jan 9, 2020

Uh oh!

SparkQA commented Jan 10, 2020

Uh oh!

HyukjinKwon commented Jan 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[SPARK-30416][SQL] Log a warning for deprecated SQL config in `set()` and `unset()` #27092

[SPARK-30416][SQL] Log a warning for deprecated SQL config in `set()` and `unset()` #27092

MaxGekk commented Jan 3, 2020 •

edited

Loading