[SPARK-37013][CORE][SQL][FOLLOWUP] Use the new error framework to throw error in `FormatString` by LuciferYang · Pull Request #34454 · apache/spark

LuciferYang · 2021-11-01T07:06:43Z

What changes were proposed in this pull request?

This is a followup of #34313. The main change of this pr is change to use the new error framework to throw error when pattern.contains("%0$") is true.

Why are the changes needed?

Use the new error framework to throw error

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins or GitHub Action

LuciferYang · 2021-11-01T07:09:05Z

let's use the new error framework to throw error in newly added code.

cc @cloud-fan , follow your advice in #34313

LuciferYang · 2021-11-01T07:14:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

@@ -1685,7 +1685,7 @@ case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.ge
 case class FormatString(children: Expression*) extends Expression with ImplicitCastInputTypes {

  require(children.nonEmpty, s"$prettyName() should take at least 1 argument")


@cloud-fan this pr only change the newly added code, If also need to change line 1687, please let me know.

cloud-fan · 2021-11-01T07:49:15Z

core/src/main/resources/error/error-classes.json

  "IF_PARTITION_NOT_EXISTS_UNSUPPORTED" : {
    "message" : [ "Cannot write, IF NOT EXISTS is not supported for table: %s" ]
  },
+  "ILLEGAL_FORMAT_ARGUMENT_INDEX" : {


Shall we have a unified error class for illegal format string? cc @karenfeng

There are a couple different ways for a string to have an illegal format, right? I'm not sure how to unify them all, but we can definitely have a more generic class here to represent that a string cannot contain a given substring, eg:

"ILLEGAL_SUBSTRING" : { "message" : [ "%s cannot contain %s." ] }

@karenfeng bf5790d fix this

@cloud-fan, does this LGTY? I don't have any more concerns.

Gentle ping @cloud-fan @dongjoon-hyun

SparkQA · 2021-11-01T08:18:22Z

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

SparkQA · 2021-11-01T09:17:23Z

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

SparkQA · 2021-11-01T10:02:18Z

Test build #144803 has finished for PR 34454 at commit af98949.

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

dongjoon-hyun

+1, LGTM.

Gentle ping @karenfeng once more for @cloud-fan 's comment, https://github.com/apache/spark/pull/34454/files#r740014071

roryqi · 2021-11-10T17:17:59Z

attern.contains("%0$") is true.
typo? 'attern' -> pattern?

LuciferYang · 2021-11-10T18:14:45Z

@jerqi thanks～already update the pr description

…LOWUP

SparkQA · 2021-11-11T04:54:19Z

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

SparkQA · 2021-11-11T05:52:11Z

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

SparkQA · 2021-11-11T06:59:23Z

Test build #145079 has finished for PR 34454 at commit bf5790d.

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

LuciferYang · 2021-11-13T04:33:14Z

Anything else needs to be changed?

LuciferYang · 2021-11-19T08:38:07Z

Gentle ping @karenfeng once more

cloud-fan · 2021-11-23T15:09:00Z

Sorry I missed the ping. Merging to master, thanks!

LuciferYang · 2021-11-24T05:35:08Z

thanks all ~

…of forbidding %0$ usage in format_string ### What changes were proposed in this pull request? Adds a legacy flag `spark.sql.legacy.allowZeroIndexInFormatString` for the breaking change introduced in #34313 and #34454 (followup). The flag is disabled by default. But when it is enabled, restore the pre-change behavior that allows the 0 based index in `format_string`. ### Why are the changes needed? The original commit is a breaking change, and breaking changes should be encouraged to add a flag to turn it off for smooth migration between versions. ### Does this PR introduce _any_ user-facing change? With the default value of the conf, there is no user-facing difference. If users turn this conf on, they can restore the pre-change behavior. ### How was this patch tested? Through unit tests. Closes #36101 from anchovYu/flags-format-string-java. Authored-by: Xinyi Yu <xinyi.yu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…of forbidding %0$ usage in format_string ### What changes were proposed in this pull request? Adds a legacy flag `spark.sql.legacy.allowZeroIndexInFormatString` for the breaking change introduced in #34313 and #34454 (followup). The flag is disabled by default. But when it is enabled, restore the pre-change behavior that allows the 0 based index in `format_string`. ### Why are the changes needed? The original commit is a breaking change, and breaking changes should be encouraged to add a flag to turn it off for smooth migration between versions. ### Does this PR introduce _any_ user-facing change? With the default value of the conf, there is no user-facing difference. If users turn this conf on, they can restore the pre-change behavior. ### How was this patch tested? Through unit tests. Closes #36101 from anchovYu/flags-format-string-java. Authored-by: Xinyi Yu <xinyi.yu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit b7af2b3) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

use new error framework

af98949

github-actions bot added CORE SQL labels Nov 1, 2021

LuciferYang commented Nov 1, 2021

View reviewed changes

cloud-fan reviewed Nov 1, 2021

View reviewed changes

LuciferYang changed the title ~~[SPARK-37013][SQL][FOLLOWUP] Use the new error framework to throw error in FormatString~~ [SPARK-37013][CORE][SQL][FOLLOWUP] Use the new error framework to throw error in FormatString Nov 1, 2021

dongjoon-hyun approved these changes Nov 7, 2021

View reviewed changes

LuciferYang added 2 commits November 11, 2021 10:48

Merge branch 'master' of github.com:apache/spark into SPARK-37013-FOL…

42eb0ea

…LOWUP

fix comments

bf5790d

cloud-fan closed this in 537b102 Nov 23, 2021

anchovYu mentioned this pull request Apr 7, 2022

[SPARK-37013][SQL][FOLLOWUP] Add legacy flag for the breaking change of forbidding %0$ usage in format_string #36101

Closed

LuciferYang deleted the SPARK-37013-FOLLOWUP branch October 22, 2023 07:43

		@@ -1685,7 +1685,7 @@ case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.ge
		case class FormatString(children: Expression*) extends Expression with ImplicitCastInputTypes {

		require(children.nonEmpty, s"$prettyName() should take at least 1 argument")

Conversation

LuciferYang commented Nov 1, 2021 • 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!

LuciferYang commented Nov 1, 2021

Uh oh!

LuciferYang Nov 1, 2021 • 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!

cloud-fan Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

karenfeng Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

LuciferYang Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

karenfeng Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

LuciferYang Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 1, 2021

Uh oh!

SparkQA commented Nov 1, 2021

Uh oh!

SparkQA commented Nov 1, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

roryqi commented Nov 10, 2021

Uh oh!

LuciferYang commented Nov 10, 2021

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

LuciferYang commented Nov 13, 2021

Uh oh!

LuciferYang commented Nov 19, 2021

Uh oh!

cloud-fan commented Nov 23, 2021

Uh oh!

LuciferYang commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LuciferYang commented Nov 1, 2021 •

edited

Loading

LuciferYang Nov 1, 2021 •

edited

Loading