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

MaxGekk · 2024-09-08T09:59:10Z

What changes were proposed in this pull request?

In the PR, I propose to detect unused error message parameters while substituting placeholders in error message formats, and raise an internal error in tests when the number of parameters is greater than the number of placeholders. That might indicate presence of unused message parameters.

Why are the changes needed?

Might happens that the passed error message parameters and placeholders in message format are not matched, and contain extra items. From the code maintainability perspective, it would be nice to detect such cases while running tests.

For example, the error message format could look like:

"CANNOT_UP_CAST_DATATYPE" : {
  "message" : [
    "Cannot up cast <expression> from <sourceType> to <targetType>.",
    "<details>"
    ],
  "sqlState" : "42846"
},

but the passed message parameters have extra parameter:

messageParameters = Map(
  "expression" -> "CAST('aaa' AS LONG)",
  "sourceType" -> "STRING",
  "targetType" -> "LONG",
  "op" -> "CAST", // unused parameter
  "details" -> "implicit cast"
))

This can happen because:

tech editor/dev forgot to mention some parameters in error message format,
wrong usage of error conditions/sub-conditions
source code became outdated
typos in error message formats

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the added test via:

$ build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.SparkThrowableSuite test

Was this patch authored or co-authored using generative AI tooling?

No.

MaxGekk · 2024-09-08T11:08:28Z

common/utils/src/main/resources/error/error-conditions.json

@@ -6222,7 +6222,7 @@
      "Detected implicit cartesian product for <joinType> join between logical plans",
      "<leftPlan>",
      "and",
-      "rightPlan",
+      "<rightPlan>",


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

MaxGekk · 2024-09-08T16:34:18Z

common/utils/src/main/resources/error/error-conditions.json

+      "Query [id = <id>, runId = <runId>, startOffset = <startOffset>, endOffset = <endOffset>] terminated with exception: <message>",
+      "<queryDebugString>"


@HeartSaVioR Could you look at the changes, please.

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.

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}

MaxGekk · 2024-09-08T19:21:50Z

@srielau Please, have a look at the PR.

MaxGekk · 2024-09-09T08:27:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/StreamExecution.scala

-            "queryDebugString" -> toDebugString(includeLogicalPlan = isInitialized),
-            "startOffset" -> getLatestExecutionContext().startOffsets.toOffsetSeq(
-              sources.toSeq, getLatestExecutionContext().offsetSeqMetadata).toString,
-            "endOffset" -> getLatestExecutionContext().endOffsets.toOffsetSeq(
-              sources.toSeq, getLatestExecutionContext().offsetSeqMetadata).toString


As @HeartSaVioR mentioned in a comment, the parameters are included in message in some circumstances.

Yeah not sure why this passed through params as we have params in constructor. Thanks for fixing this!

…-params

srielau · 2024-09-09T15:05:22Z

common/utils/src/main/resources/error/error-conditions.json

@@ -7827,7 +7827,7 @@
  },
  "_LEGACY_ERROR_TEMP_3055" : {


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

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

common/utils/src/main/resources/error/error-conditions.json

MaxGekk · 2024-09-09T19:42:54Z

@HeartSaVioR Do you have any objections for the changes?

allisonwang-db · 2024-09-09T20:00:25Z

common/utils/src/main/resources/error/error-conditions.json

@@ -904,7 +904,7 @@
      },
      "NON_STRING_TYPE" : {
        "message" : [
-          "all arguments must be strings."
+          "all arguments of the function <funcName> must be strings."


Good catch!

MaxGekk · 2024-09-10T06:05:20Z

Merging to master. Thank you, @HeartSaVioR @srielau @allisonwang-db @HyukjinKwon for review.

…Class` in `SparkThrowableSuite` and in `DateTimeFormatterHelperSuite` ### What changes were proposed in this pull request? In the PR, I propose to use `condition` instead of `errorClass` in two test suites: - SparkThrowableSuite - DateTimeFormatterHelperSuite ### Why are the changes needed? Because the changes from the PR #48027 conflict to #48058 and #48026, and tests in #48027 were passed earlier than the last PRs were merged to master. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By compiling and running the following tests locally: ``` $ build/sbt "test:testOnly *org.apache.spark.sql.catalyst.util.DateTimeFormatterHelperSuite" $ build/sbt "test:testOnly *SparkThrowableSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48061 from MaxGekk/fix-missing-errorClass. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? In the PR, I propose to detect unused error message parameters while substituting placeholders in error message formats, and raise an internal error in tests when the number of parameters is greater than the number of placeholders. That might indicate presence of unused message parameters. ### Why are the changes needed? Might happens that the passed error message parameters and placeholders in message format are not matched, and contain extra items. From the code maintainability perspective, it would be nice to detect such cases while running tests. For example, the error message format could look like: ```json "CANNOT_UP_CAST_DATATYPE" : { "message" : [ "Cannot up cast <expression> from <sourceType> to <targetType>.", "<details>" ], "sqlState" : "42846" }, ``` but the passed message parameters have extra parameter: ```scala messageParameters = Map( "expression" -> "CAST('aaa' AS LONG)", "sourceType" -> "STRING", "targetType" -> "LONG", "op" -> "CAST", // unused parameter "details" -> "implicit cast" )) ``` This can happen because: - tech editor/dev forgot to mention some parameters in error message format, - wrong usage of error conditions/sub-conditions - source code became outdated - typos in error message formats ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By running the added test via: ``` $ build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.SparkThrowableSuite test ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48026 from MaxGekk/detect-unused-error-params. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…Class` in `SparkThrowableSuite` and in `DateTimeFormatterHelperSuite` ### What changes were proposed in this pull request? In the PR, I propose to use `condition` instead of `errorClass` in two test suites: - SparkThrowableSuite - DateTimeFormatterHelperSuite ### Why are the changes needed? Because the changes from the PR apache#48027 conflict to apache#48058 and apache#48026, and tests in apache#48027 were passed earlier than the last PRs were merged to master. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By compiling and running the following tests locally: ``` $ build/sbt "test:testOnly *org.apache.spark.sql.catalyst.util.DateTimeFormatterHelperSuite" $ build/sbt "test:testOnly *SparkThrowableSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48061 from MaxGekk/fix-missing-errorClass. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? In the PR, I propose to detect unused error message parameters while substituting placeholders in error message formats, and raise an internal error in tests when the number of parameters is greater than the number of placeholders. That might indicate presence of unused message parameters. ### Why are the changes needed? Might happens that the passed error message parameters and placeholders in message format are not matched, and contain extra items. From the code maintainability perspective, it would be nice to detect such cases while running tests. For example, the error message format could look like: ```json "CANNOT_UP_CAST_DATATYPE" : { "message" : [ "Cannot up cast <expression> from <sourceType> to <targetType>.", "<details>" ], "sqlState" : "42846" }, ``` but the passed message parameters have extra parameter: ```scala messageParameters = Map( "expression" -> "CAST('aaa' AS LONG)", "sourceType" -> "STRING", "targetType" -> "LONG", "op" -> "CAST", // unused parameter "details" -> "implicit cast" )) ``` This can happen because: - tech editor/dev forgot to mention some parameters in error message format, - wrong usage of error conditions/sub-conditions - source code became outdated - typos in error message formats ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By running the added test via: ``` $ build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.SparkThrowableSuite test ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48026 from MaxGekk/detect-unused-error-params. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…Class` in `SparkThrowableSuite` and in `DateTimeFormatterHelperSuite` ### What changes were proposed in this pull request? In the PR, I propose to use `condition` instead of `errorClass` in two test suites: - SparkThrowableSuite - DateTimeFormatterHelperSuite ### Why are the changes needed? Because the changes from the PR apache#48027 conflict to apache#48058 and apache#48026, and tests in apache#48027 were passed earlier than the last PRs were merged to master. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By compiling and running the following tests locally: ``` $ build/sbt "test:testOnly *org.apache.spark.sql.catalyst.util.DateTimeFormatterHelperSuite" $ build/sbt "test:testOnly *SparkThrowableSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48061 from MaxGekk/fix-missing-errorClass. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

MaxGekk added 2 commits September 8, 2024 11:43

Add a test

77971ff

Check and raise exception

89c2062

github-actions bot added the CORE label Sep 8, 2024

Fix tests

a3699f0

MaxGekk mentioned this pull request Sep 8, 2024

[SPARK-43299][FOLLOWUP][CONNECT][SS] Followup on StreamingQueryExceptions #44306

Closed

MaxGekk added 3 commits September 8, 2024 12:59

Fix error-conditions.json

337b2a9

Remove unused parameters for STREAM_FAILED

3a3f6e3

Revert StreamExecution.scala

ff8aedb

MaxGekk commented Sep 8, 2024

View reviewed changes

MaxGekk added 3 commits September 8, 2024 16:01

Update error format of NON_STRING_TYPE

0b25486

Add parameters to the error format of STREAM_FAILED

05ef7f5

Fix the error format of _LEGACY_ERROR_TEMP_3055

bf00d0d

MaxGekk changed the title ~~[WIP][SPARK-49538][SQL][TESTS] Detect unused error message parameters~~ [SPARK-49538][SQL][TESTS] Detect unused error message parameters Sep 8, 2024

MaxGekk marked this pull request as ready for review September 8, 2024 16:31

MaxGekk commented Sep 8, 2024

View reviewed changes

Improve the error message

b24dae2

HyukjinKwon approved these changes Sep 9, 2024

View reviewed changes

Revert changes for STREAM_FAILED

55af118

github-actions bot added SQL STRUCTURED STREAMING labels Sep 9, 2024

Remove checking of unused parameters

bc72974

github-actions bot added the CONNECT label Sep 9, 2024

MaxGekk commented Sep 9, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into detect-unused-error…

07df49a

…-params

srielau reviewed Sep 9, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Show resolved Hide resolved

srielau approved these changes Sep 9, 2024

View reviewed changes

allisonwang-db approved these changes Sep 9, 2024

View reviewed changes

MaxGekk closed this in 5f2d839 Sep 10, 2024

MaxGekk mentioned this pull request Sep 10, 2024

[SPARK-49579][SQL][TESTS][FOLLOWUP] Use condition instead of errorClass in SparkThrowableSuite and in DateTimeFormatterHelperSuite #48061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MaxGekk commented Sep 8, 2024 •

edited

Loading

MaxGekk Sep 8, 2024

MaxGekk Sep 8, 2024

HeartSaVioR Sep 9, 2024 •

edited

Loading

MaxGekk Sep 9, 2024 •

edited

Loading

MaxGekk commented Sep 8, 2024

MaxGekk Sep 9, 2024

HeartSaVioR Sep 12, 2024

srielau Sep 9, 2024

MaxGekk Sep 9, 2024

MaxGekk commented Sep 9, 2024

allisonwang-db Sep 9, 2024

MaxGekk commented Sep 10, 2024

		"Query [id = <id>, runId = <runId>, startOffset = <startOffset>, endOffset = <endOffset>] terminated with exception: <message>",
		"<queryDebugString>"

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

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

Conversation

MaxGekk commented Sep 8, 2024 • edited Loading

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?

Was this patch authored or co-authored using generative AI tooling?

MaxGekk Sep 8, 2024

Choose a reason for hiding this comment

MaxGekk Sep 8, 2024

Choose a reason for hiding this comment

HeartSaVioR Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

MaxGekk Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

MaxGekk commented Sep 8, 2024

MaxGekk Sep 9, 2024

Choose a reason for hiding this comment

HeartSaVioR Sep 12, 2024

Choose a reason for hiding this comment

srielau Sep 9, 2024

Choose a reason for hiding this comment

MaxGekk Sep 9, 2024

Choose a reason for hiding this comment

MaxGekk commented Sep 9, 2024

allisonwang-db Sep 9, 2024

Choose a reason for hiding this comment

MaxGekk commented Sep 10, 2024

MaxGekk commented Sep 8, 2024 •

edited

Loading

HeartSaVioR Sep 9, 2024 •

edited

Loading

MaxGekk Sep 9, 2024 •

edited

Loading