Skip to content

[SPARK-49538][SQL][TESTS] Detect unused error message parameters #48026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 13 commits into from
Closed

[SPARK-49538][SQL][TESTS] Detect unused error message parameters #48026

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
77971ff
Add a test
MaxGekk Sep 8, 2024
89c2062
Check and raise exception
MaxGekk Sep 8, 2024
a3699f0
Fix tests
MaxGekk Sep 8, 2024
337b2a9
Fix error-conditions.json
MaxGekk Sep 8, 2024
3a3f6e3
Remove unused parameters for STREAM_FAILED
MaxGekk Sep 8, 2024
ff8aedb
Revert StreamExecution.scala
MaxGekk Sep 8, 2024
0b25486
Update error format of NON_STRING_TYPE
MaxGekk Sep 8, 2024
05ef7f5
Add parameters to the error format of STREAM_FAILED
MaxGekk Sep 8, 2024
bf00d0d
Fix the error format of _LEGACY_ERROR_TEMP_3055
MaxGekk Sep 8, 2024
b24dae2
Improve the error message
MaxGekk Sep 8, 2024
55af118
Revert changes for STREAM_FAILED
MaxGekk Sep 9, 2024
bc72974
Remove checking of unused parameters
MaxGekk Sep 9, 2024
07df49a
Merge remote-tracking branch 'origin/master' into detect-unused-error…
MaxGekk Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions common/utils/src/main/resources/error/error-conditions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@
},
"NON_STRING_TYPE" : {
"message" : [
"all arguments must be strings."
"all arguments of the function <funcName> must be strings."

@allisonwang-db allisonwang-db Sep 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch!

]
},
"NULL_TYPE" : {
Expand Down Expand Up @@ -4271,7 +4271,8 @@
},
"STREAM_FAILED" : {
"message" : [
"Query [id = <id>, runId = <runId>] terminated with exception: <message>"
"Query [id = <id>, runId = <runId>, startOffset = <startOffset>, endOffset = <endOffset>] terminated with exception: <message>",
"<queryDebugString>"

@MaxGekk MaxGekk Sep 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@HeartSaVioR Could you look at the changes, please.

@HeartSaVioR HeartSaVioR Sep 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be slightly messed up -

https://github.com/apache/spark/blob/master/common/utils/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryException.scala

This class overrides getMessage and handles queryDebugString separately. So it's expected to not have queryDebugString here.

What I'm confused is why we have a case where queryDebugString is passed over message parameters, since we have constructors which directly receive queryDebugString.

@MaxGekk MaxGekk Sep 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So it's expected to not have queryDebugString here.

@HeartSaVioR ok, let me remove it from message parameters and from the error format. How about other 2 unused parameters: startOffset and endOffset. Does queryDebugString include the info, it seems?

  private def toDebugString(includeLogicalPlan: Boolean): String = {
    val debugString =
      s"""|=== Streaming Query ===
          |Identifier: $prettyIdString
          |Current Committed Offsets: ${getLatestExecutionContext().startOffsets}
          |Current Available Offsets: ${getLatestExecutionContext().endOffsets}

],
"sqlState" : "XXKST"
},
Expand Down Expand Up @@ -6222,7 +6223,7 @@
"Detected implicit cartesian product for <joinType> join between logical plans",
"<leftPlan>",
"and",
"rightPlan",
"<rightPlan>",

@MaxGekk MaxGekk Sep 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The purpose of this PR is to detect mistakes like this.

"Join condition is missing or trivial.",
"Either: use the CROSS JOIN syntax to allow cartesian products between these relations, or: enable implicit cartesian products by setting the configuration variable spark.sql.crossJoin.enabled=true."
]
Expand Down Expand Up @@ -7827,7 +7828,7 @@
},
"_LEGACY_ERROR_TEMP_3055" : {

@srielau srielau Sep 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: If we're already touching this _LEGACY, could we just name it? (is this an internal error??)

@MaxGekk MaxGekk Sep 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I opened the starter task https://issues.apache.org/jira/browse/SPARK-49549 @mitkedb FYI.

"message" : [
"ScalarFunction '<scalarFunc.name>' neither implement magic method nor override 'produceResult'"
"ScalarFunction <scalarFunc> neither implement magic method nor override 'produceResult'"
Comment thread
MaxGekk marked this conversation as resolved.
]
},
"_LEGACY_ERROR_TEMP_3056" : {
Expand Down
14 changes: 12 additions & 2 deletions common/utils/src/main/scala/org/apache/spark/ErrorClassesJSONReader.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package org.apache.spark

import java.net.URL

import scala.collection.immutable.Map
import scala.jdk.CollectionConverters._

import com.fasterxml.jackson.annotation.JsonIgnore
Expand Down Expand Up @@ -52,7 +51,7 @@ class ErrorClassesJsonReader(jsonFileURLs: Seq[URL]) {
val sub = new StringSubstitutor(sanitizedParameters.asJava)
sub.setEnableUndefinedVariableException(true)
sub.setDisableSubstitutionInValues(true)
try {
val errorMessage = try {
sub.replace(ErrorClassesJsonReader.TEMPLATE_REGEX.replaceAllIn(
messageTemplate, "\\$\\{$1\\}"))
} catch {
Expand All @@ -61,6 +60,17 @@ class ErrorClassesJsonReader(jsonFileURLs: Seq[URL]) {
s"MessageTemplate: $messageTemplate, " +
s"Parameters: $messageParameters", i)
}
if (util.SparkEnvUtils.isTesting) {
val placeHoldersNum = ErrorClassesJsonReader.TEMPLATE_REGEX.findAllIn(messageTemplate).length
if (placeHoldersNum < sanitizedParameters.size) {
throw SparkException.internalError(
s"Found unused message parameters for the error class $errorClass. " +
s"Its error message format has $placeHoldersNum place holders, " +
s"but the passed message parameters map has ${sanitizedParameters.size} items. " +
"Consider to add place holders to the error format or remove unused message parameters.")
}
}
errorMessage
}

def getMessageParameters(errorClass: String): Seq[String] = {
Expand Down
33 changes: 25 additions & 8 deletions core/src/test/scala/org/apache/spark/SparkThrowableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,6 @@ class SparkThrowableSuite extends SparkFunSuite {
}
assert(e.getErrorClass === "INTERNAL_ERROR")
assert(e.getMessageParameters().get("message").contains("Undefined error message parameter"))

// Does not fail with too many args (expects 0 args)
assert(getMessage("DIVIDE_BY_ZERO", Map("config" -> "foo", "a" -> "bar")) ==
"[DIVIDE_BY_ZERO] Division by zero. " +
"Use `try_divide` to tolerate divisor being 0 and return NULL instead. " +
"If necessary set foo to \"false\" " +
"to bypass this error. SQLSTATE: 22012")
}

test("Error message is formatted") {
Expand Down Expand Up @@ -504,7 +497,7 @@ class SparkThrowableSuite extends SparkFunSuite {
|{
| "MISSING_PARAMETER" : {
| "message" : [
| "Parameter ${param} is missing."
| "Parameter <param> is missing."
| ]
| }
|}
Expand All @@ -517,4 +510,28 @@ class SparkThrowableSuite extends SparkFunSuite {
assert(errorMessage.contains("Parameter null is missing."))
}
}

test("detect unused message parameters") {
checkError(
exception = intercept[SparkException] {
SparkThrowableHelper.getMessage(
errorClass = "CANNOT_UP_CAST_DATATYPE",
messageParameters = Map(
"expression" -> "CAST('aaa' AS LONG)",
"sourceType" -> "STRING",
"targetType" -> "LONG",
"op" -> "CAST", // unused parameter
"details" -> "implicit cast"
))
},
errorClass = "INTERNAL_ERROR",
parameters = Map(
"message" ->
("Found unused message parameters for the error class CANNOT_UP_CAST_DATATYPE. " +
"Its error message format has 4 place holders, but the passed message parameters map " +
"has 5 items. Consider to add place holders to the error format or " +
"remove unused message parameters.")
)
)
}
}