Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Sep 8, 2022

What changes were proposed in this pull request?

In the PR, I propose to change the type of messageParameters passed to Spark exception (via constructors) from array of strings to map of pair of strings. The map contains parameters names from error-classes.json as keys, and parameters values as map's values. While formatting error messages using the templates from error-classes.json, the formatter can fail w/ an internal error if it cannot find a parameter in the map passed to the current exception. The map of message parameters can contain more elements then used by the template.

Before:

new AnalysisException(
  errorClass = "UNSUPPORTED_GENERATOR",
  errorSubClass = "MULTI_GENERATOR",
  messageParameters = Array(clause, generators.size.toString, generators.map(toSQLExpr).mkString(", ")))

After:

new AnalysisException(
  errorClass = "UNSUPPORTED_GENERATOR",
  errorSubClass = "MULTI_GENERATOR",
  messageParameters = Map(
    "clause" -> clause,
    "num" -> generators.size.toString,
    "generators" -> generators.map(toSQLExpr).mkString(", ")))

Why are the changes needed?

At the moment, the order of error message parameters cannot be changed in error-classes.json independently from the source code where the parameters are passed as arrays. After the changes, the tech editors/users can change the error messages including the order of params or skip some params w/ modifying Spark's code base (including tests).

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

By running the modified test suites:

$ build/sbt "test:testOnly *ErrorParserSuite"
$ build/sbt "test:testOnly *AnalysisErrorSuite"
$ build/sbt "test:testOnly *AnalysisSuite
$ build/sbt "test:testOnly *ResolveSubquerySuite"
$ build/sbt "test:testOnly *V2OverwriteByExpressionANSIAnalysisSuite"
$ build/sbt "test:testOnly *TimeWindowSuite"
$ build/sbt "test:testOnly *DataSourceV2SQLSuiteV1Filter"
$ build/sbt "test:testOnly *DataFrameSuite"
$ build/sbt "test:testOnly *JsonV1Suite"

@github-actions github-actions bot added the CORE label Sep 8, 2022
@github-actions github-actions bot added the SQL label Sep 8, 2022
@github-actions github-actions bot added the BUILD label Sep 9, 2022
@MaxGekk MaxGekk changed the title [WIP][SQL] Pass error message parameters to exceptions as a map [WIP][SPARK-40400][SQL] Pass error message parameters to exceptions as a map Sep 9, 2022
@MaxGekk MaxGekk changed the title [WIP][SPARK-40400][SQL] Pass error message parameters to exceptions as a map [SPARK-40400][SQL] Pass error message parameters to exceptions as a map Sep 10, 2022
@MaxGekk MaxGekk changed the title [SPARK-40400][SQL] Pass error message parameters to exceptions as a map [SPARK-40400][SQL] Pass error message parameters to exceptions as maps Sep 10, 2022
@MaxGekk MaxGekk marked this pull request as ready for review September 10, 2022 06:11
@MaxGekk
Copy link
Member Author

MaxGekk commented Sep 10, 2022

@srielau @anchovYu Could you take a look at the PR, please.

@MaxGekk
Copy link
Member Author

MaxGekk commented Sep 12, 2022

@cloud-fan @gengliangwang Could you review this PR, please.

with SparkThrowable {

override def getMessageParameters: Array[String] = {
SparkThrowableHelper.getMessageParameters(errorClass, errorSubClass.orNull, messageParameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxGekk @srielau This line shows up many times in this PR.
Do we consider further refactoring the parameters in SparkThrowable? E.g. combine String[] getMessageParameters and String[] getParameterNames into Map<String, String> getMessageParameters

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. These 2 methods were added by #36693 and not released yet. We can still change it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's do that in a separate PR.

@MaxGekk
Copy link
Member Author

MaxGekk commented Sep 13, 2022

Merging to master. Thank you, @srielau @gengliangwang @cloud-fan for review.

@MaxGekk MaxGekk closed this in 96ed6dc Sep 13, 2022
"proposal" ->
"`__auto_generated_subquery_name`.`c1`, `__auto_generated_subquery_name`.`c2`"),
context = ExpectedContext(
fragment = insert,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxGekk is it a bug? The error context should be c3 in this case, instead of the INSERT statement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be c3. The PR #37861 changes this in OSS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants