[SPARK-33591][SQL][FOLLOWUP] Add legacy config for recognizing null partition spec values #31421

gengliangwang · 2021-02-01T17:28:21Z

What changes were proposed in this pull request?

This is a follow up for #30538.
It adds a legacy conf spark.sql.legacy.parseNullPartitionSpecAsStringLiteral in case users wants the legacy behavior.
It also adds document for the behavior change.

Why are the changes needed?

In case users want the legacy behavior, they can set spark.sql.legacy.parseNullPartitionSpecAsStringLiteral as true.

Does this PR introduce any user-facing change?

Yes, adding a legacy configuration to restore the old behavior.

How was this patch tested?

Unit test.

gengliangwang · 2021-02-01T17:28:37Z

cc @cloud-fan @HyukjinKwon @MaxGekk

MaxGekk · 2021-02-01T18:04:14Z

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

    }
  }
+
+  test("SPARK-33591: null as string partition literal value 'null' after setting legacy conf") {


Could you move this test to org.apache.spark.sql.execution.command.ShowPartitionsSuiteBase. It should pass for DS v2 too.

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

MaxGekk · 2021-02-01T18:10:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+      val processNullLiteral =
+        !conf.getConf(SQLConf.LEGACY_PARSE_NULL_PARTITION_SPEC_AS_STRING_LITERAL)


Could you move the config read out of map(). Don't need to re-read it every partition part.

SparkQA · 2021-02-01T22:37:51Z

Test build #134741 has finished for PR 31421 at commit b062f8b.

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

…/v1/ShowPartitionsSuite.scala Co-authored-by: Maxim Gekk <[email protected]>

gengliangwang · 2021-02-02T04:04:20Z

@MaxGekk Thanks for the suggestions. I have addressed all of them.

SparkQA · 2021-02-02T04:40:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39342/

SparkQA · 2021-02-02T05:08:37Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39342/

HyukjinKwon · 2021-02-02T07:17:46Z

Merged to master.

@gengliangwang would you mind changing the version and making backporting PRs please?

cloud-fan · 2021-02-02T07:36:23Z

docs/sql-migration-guide.md


  - In Spark 3.2, the auto-generated `Cast` (such as those added by type coercion rules) will be stripped when generating column alias names. E.g., `sql("SELECT floor(1)").columns` will be `FLOOR(1)` instead of `FLOOR(CAST(1 AS DOUBLE))`.

+  - In Spark 3.2, a null partition value is parsed as it is. In Spark 3.1 or earlier, it is parsed as a string literal of its text representation, e.g., string "null". To restore the legacy behavior, you can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true.


Let's be more specific

In Spark 3.2, `PARTITION(col=null)` is always parsed as a null literal in the partition spec. In Spark 3.1 or earlier, it is parsed as a string literal ..., if the partition column is string type. To restore the legacy behavior, ...

cloud-fan · 2021-02-02T07:39:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

   */
-  protected def visitStringConstant(ctx: ConstantContext): String = withOrigin(ctx) {
+  protected def visitStringConstant(
+    ctx: ConstantContext,


nit: 4 spaces indentation

cloud-fan · 2021-02-02T07:39:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

   */
  override def visitPartitionSpec(
      ctx: PartitionSpecContext): Map[String, Option[String]] = withOrigin(ctx) {
+    val processNullLiteral =


this name is weird, how about legacyNullAsString?

cloud-fan · 2021-02-02T07:40:12Z

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

+  val LEGACY_PARSE_NULL_PARTITION_SPEC_AS_STRING_LITERAL =
+    buildConf("spark.sql.legacy.parseNullPartitionSpecAsStringLiteral")
+      .internal()
+      .doc("If it is set to true, a null partition value is parsed as a string literal of its " +


ditto, make it more specific.

cloud-fan · 2021-02-02T07:40:33Z

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

+      .doc("If it is set to true, a null partition value is parsed as a string literal of its " +
+        "text representation, e.g., string 'null'. Otherwise, null partition values are parsed " +
+        "as they are.")
+      .version("3.2.0")


the version should be 3.1.1, as we are going to backport this.

should be 3.0.2 actually.

Ah I see, then how should we place it in the migration guide? Repeat it 3 times in 3.0.2, 3.1.1 and 3.2.0 sections?

I think we have documented only in 3.0.2 so far in such cases ..

cloud-fan · 2021-02-02T07:42:18Z

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

+        sql(s"CREATE TABLE $t (col1 INT, p1 STRING) $defaultUsing PARTITIONED BY (p1)")
+        sql(s"INSERT INTO TABLE $t PARTITION (p1 = null) SELECT 0")
+        runShowPartitionsSql(s"SHOW PARTITIONS $t", Row("p1=null") :: Nil)
+        runShowPartitionsSql(s"SHOW PARTITIONS $t PARTITION (p1 = null)", Row("p1=null") :: Nil)


how does this show the null string behavior? shall we just test a simple table scan operation?

gengliangwang · 2021-02-02T08:24:45Z

@cloud-fan Thanks for the comments. I have addressed them in #31434

SparkQA · 2021-02-02T08:32:38Z

Test build #134756 has finished for PR 31421 at commit 971ccbd.

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

…l.legacy.parseNullPartitionSpecAsStringLiteral` ### What changes were proposed in this pull request? Correct the version of SQL configuration `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` from 3.2.0 to 3.0.2. Also, revise the documentation and test case. ### Why are the changes needed? The release version in #31421 was wrong. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit tests Closes #31434 from gengliangwang/reviseVersion. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…artition spec values This is a follow up for apache#30538. It adds a legacy conf `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` in case users wants the legacy behavior. It also adds document for the behavior change. In case users want the legacy behavior, they can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true. Yes, adding a legacy configuration to restore the old behavior. Unit test. Closes apache#31421 from gengliangwang/legacyNullStringConstant. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…l.legacy.parseNullPartitionSpecAsStringLiteral` Correct the version of SQL configuration `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` from 3.2.0 to 3.0.2. Also, revise the documentation and test case. The release version in apache#31421 was wrong. No Unit tests Closes apache#31434 from gengliangwang/reviseVersion. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…artition spec values This is a follow up for apache#30538. It adds a legacy conf `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` in case users wants the legacy behavior. It also adds document for the behavior change. In case users want the legacy behavior, they can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true. Yes, adding a legacy configuration to restore the old behavior. Unit test. Closes apache#31421 from gengliangwang/legacyNullStringConstant. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…l.legacy.parseNullPartitionSpecAsStringLiteral` Correct the version of SQL configuration `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` from 3.2.0 to 3.0.2. Also, revise the documentation and test case. The release version in apache#31421 was wrong. No Unit tests Closes apache#31434 from gengliangwang/reviseVersion. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…ull partition spec values ### What changes were proposed in this pull request? This PR is to backport #31421 and #31421 to branch 3.0 This is a follow up for #30538. It adds a legacy conf `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` in case users wants the legacy behavior. It also adds document for the behavior change. ### Why are the changes needed? In case users want the legacy behavior, they can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true. ### Does this PR introduce _any_ user-facing change? Yes, adding a legacy configuration to restore the old behavior. ### How was this patch tested? Unit test. Closes #31441 from gengliangwang/backportLegacyConf3.0. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…ull partition spec values ### What changes were proposed in this pull request? This PR is to backport #31421 and #31434 to branch 3.1 This is a follow up for #30538. It adds a legacy conf `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` in case users wants the legacy behavior. It also adds document for the behavior change. ### Why are the changes needed? In case users want the legacy behavior, they can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true. ### Does this PR introduce _any_ user-facing change? Yes, adding a legacy configuration to restore the old behavior. ### How was this patch tested? Unit test. Closes #31439 from gengliangwang/backportLegacyConf3.1. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…artition spec values ### What changes were proposed in this pull request? This is a follow up for apache#30538. It adds a legacy conf `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` in case users wants the legacy behavior. It also adds document for the behavior change. ### Why are the changes needed? In case users want the legacy behavior, they can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true. ### Does this PR introduce _any_ user-facing change? Yes, adding a legacy configuration to restore the old behavior. ### How was this patch tested? Unit test. Closes apache#31421 from gengliangwang/legacyNullStringConstant. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…l.legacy.parseNullPartitionSpecAsStringLiteral` ### What changes were proposed in this pull request? Correct the version of SQL configuration `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` from 3.2.0 to 3.0.2. Also, revise the documentation and test case. ### Why are the changes needed? The release version in apache#31421 was wrong. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit tests Closes apache#31434 from gengliangwang/reviseVersion. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

gengliangwang added 2 commits February 2, 2021 00:12

add legacy flag

1c5c3bf

revise

b062f8b

github-actions bot added DOCS SQL labels Feb 1, 2021

MaxGekk reviewed Feb 1, 2021

View reviewed changes

gengliangwang and others added 2 commits February 1, 2021 19:43

Update sql/core/src/test/scala/org/apache/spark/sql/execution/command…

747ac33

…/v1/ShowPartitionsSuite.scala Co-authored-by: Maxim Gekk <[email protected]>

address comments

971ccbd

HyukjinKwon approved these changes Feb 2, 2021

View reviewed changes

HyukjinKwon closed this in 521397f Feb 2, 2021

gengliangwang mentioned this pull request Feb 2, 2021

[SPARK-33591][SQL][FOLLOW-UP] Revise the version and doc of spark.sql.legacy.parseNullPartitionSpecAsStringLiteral #31434

Closed

cloud-fan reviewed Feb 2, 2021

View reviewed changes

gengliangwang mentioned this pull request Feb 2, 2021

[SPARK-33591][3.1][SQL][FOLLOWUP] Add legacy config for recognizing null partition spec values #31439

Closed

gengliangwang mentioned this pull request Feb 2, 2021

[SPARK-33591][3.0][SQL][FOLLOWUP] Add legacy config for recognizing null partition spec values #31441

Closed

		val processNullLiteral =
		!conf.getConf(SQLConf.LEGACY_PARSE_NULL_PARTITION_SPEC_AS_STRING_LITERAL)


		- In Spark 3.2, the auto-generated `Cast` (such as those added by type coercion rules) will be stripped when generating column alias names. E.g., `sql("SELECT floor(1)").columns` will be `FLOOR(1)` instead of `FLOOR(CAST(1 AS DOUBLE))`.

		- In Spark 3.2, a null partition value is parsed as it is. In Spark 3.1 or earlier, it is parsed as a string literal of its text representation, e.g., string "null". To restore the legacy behavior, you can set `spark.sql.legacy.parseNullPartitionSpecAsStringLiteral` as true.

[SPARK-33591][SQL][FOLLOWUP] Add legacy config for recognizing null partition spec values #31421

[SPARK-33591][SQL][FOLLOWUP] Add legacy config for recognizing null partition spec values #31421

Uh oh!

Conversation

gengliangwang commented Feb 1, 2021

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!

gengliangwang commented Feb 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 1, 2021

Uh oh!

gengliangwang commented Feb 2, 2021

Uh oh!

SparkQA commented Feb 2, 2021

Uh oh!

SparkQA commented Feb 2, 2021

Uh oh!

HyukjinKwon commented Feb 2, 2021

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!

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!

gengliangwang commented Feb 2, 2021

Uh oh!

SparkQA commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants