[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERROR_TEMP_3055, 3146 #48288

mrk-andreev · 2024-09-27T20:14:54Z

What changes were proposed in this pull request?

Choose a proper name for the error conditions _LEGACY_ERROR_TEMP_3055 and _LEGACY_ERROR_TEMP_3146 defined in core/src/main/resources/error/error-conditions.json. The name should be short but complete (look at the example in error-conditions.json).

Add a test which triggers the error from user code if such test still doesn't exist. Check exception fields by using checkError(). The last function checks valuable error fields only, and avoids dependencies from error text message. In this way, tech editors can modify error format in error-conditions.json, and don't worry of Spark's internal tests. Migrate other tests that might trigger the error onto checkError().

Why are the changes needed?

This changes needed because spark 4 introduce new approach with user friendly error messages.

Does this PR introduce any user-facing change?

Yes, if user's code depends on error condition names.

How was this patch tested?

Unit tests

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

No

MaxGekk · 2024-10-02T15:34:13Z

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

@@ -182,7 +182,7 @@ object V2ExpressionUtils extends SQLConfHelper with Logging {
            ApplyFunctionExpression(scalarFunc, arguments)
          case _ =>
            throw new AnalysisException(
-              errorClass = "_LEGACY_ERROR_TEMP_3055",
+              errorClass = "SCALAR_FUNCTION_NOT_FULLY_IMPLEMENTED",
              messageParameters = Map("scalarFunc" -> scalarFunc.name()))


Since scalarFunc.name is an id, let's quote it by toSQLId()

Replaced with

messageParameters = Map("scalarFunc" -> toSQLId(scalarFunc.name())))

MaxGekk · 2024-10-02T15:38:01Z

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

+  },
+  "SCALAR_FUNCTION_NOT_FULLY_IMPLEMENTED" : {
+    "message" : [
+      "ScalarFunction <scalarFunc> neither implements magic method nor override 'produceResult'"


I think we should explicitly say what we expect: a method with the name produceResult which accepts a parameter of the type InternalRow

Replaced with

"ScalarFunction <scalarFunc> not implements or overrides method 'produceResult(InternalRow)'."

MaxGekk · 2024-10-02T15:38:45Z

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

@@ -3975,6 +3975,16 @@
    ],
    "sqlState" : "21000"
  },
+  "SCALAR_FUNCTION_NOT_COMPATIBLE" : {
+    "message" : [
+      "Cannot find a compatible ScalarFunction#produceResult"


Can we suggest users how to fix the issue?

Replaced with

"ScalarFunction <scalarFunc> not overrides method 'produceResult(InternalRow)' with custom implementation."

MaxGekk · 2024-10-02T15:41:30Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2FunctionSuite.scala

-      condition = "_LEGACY_ERROR_TEMP_3055",
-      parameters = Map("scalarFunc" -> "long_add_mismatch_magic"),
+      condition = "SCALAR_FUNCTION_NOT_FULLY_IMPLEMENTED",
+      parameters = Map(),


there is no scalarFunc parameter?

Fixed with

parameters = Map("scalarFunc" -> "`long_add_mismatch_magic`"),

MaxGekk · 2024-10-09T08:57:31Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/functions/ScalarFunction.java

@@ -149,7 +149,7 @@ public interface ScalarFunction<R> extends BoundFunction {
   * @return a result value
   */
  default R produceResult(InternalRow input) {
-    throw new SparkUnsupportedOperationException("_LEGACY_ERROR_TEMP_3146");
+    throw new SparkUnsupportedOperationException("SCALAR_FUNCTION_NOT_COMPATIBLE");


The error condition requires the parameter scalarFunc. Could you pass it here, please.

Fixed with

throw new SparkUnsupportedOperationException( "SCALAR_FUNCTION_NOT_COMPATIBLE", Map.of("scalarFunc", QuotingUtils.quoteIdentifier(name())) );

I used QuotingUtils.quoteIdentifier instead toSQLId because this method is unavaible from this location.

MaxGekk

LGTM except of a comment. Also update PR's description:

remove the link to JIRA ticket
error-classes.json -> error-conditions.json

MaxGekk · 2024-10-09T10:33:04Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/functions/ScalarFunction.java

+            "SCALAR_FUNCTION_NOT_COMPATIBLE",
+            Map.of("scalarFunc", QuotingUtils.quoteIdentifier(name()))


Could you fix indentation. Should be 2 gaps.

Could you fix indentation. Should be 2 gaps.

Fixed

LGTM except of a comment. Also update PR's description:
remove the link to JIRA ticket
error-classes.json -> error-conditions.json

Fixed

MaxGekk · 2024-10-09T14:31:27Z

@mrk-andreev Could you fix the failed tests:

[info] - Error conditions are correctly formatted *** FAILED *** (103 milliseconds)
[info]   "...023"
[info]     },
[info]     "SCALAR_[FUNCTION_NOT_COMPATIBLE" : {
[info]       "message" : [
[info]         "ScalarFunction <scalarFunc> not overrides method 'produceResult(InternalRow)' with custom implementation."
[info]       ],
[info]       "sqlState" : "42K0O"
[info]     },

You should reformat error-conditions.json by the command:

$ SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "core/testOnly *SparkThrowableSuite -- -t \"Error conditions are correctly formatted\""

…R_TEMP_3055, 3146

MaxGekk · 2024-10-09T18:14:12Z

Waiting for CI. @mrk-andreev Could you re-trigger the failed GitHub action:
Run / Build modules: api, catalyst, hive-thriftserver

mrk-andreev · 2024-10-09T21:15:50Z

@MaxGekk fixed.

MaxGekk · 2024-10-10T06:58:50Z

+1, LGTM. Merging to master.
Thank you, @mrk-andreev.

…R_TEMP_3055, 3146 ### What changes were proposed in this pull request? Choose a proper name for the error conditions _LEGACY_ERROR_TEMP_3055 and _LEGACY_ERROR_TEMP_3146 defined in core/src/main/resources/error/error-conditions.json. The name should be short but complete (look at the example in error-conditions.json). Add a test which triggers the error from user code if such test still doesn't exist. Check exception fields by using checkError(). The last function checks valuable error fields only, and avoids dependencies from error text message. In this way, tech editors can modify error format in error-conditions.json, and don't worry of Spark's internal tests. Migrate other tests that might trigger the error onto checkError(). ### Why are the changes needed? This changes needed because spark 4 introduce new approach with user friendly error messages. ### Does this PR introduce _any_ user-facing change? Yes, if user's code depends on error condition names. ### How was this patch tested? Unit tests ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#48288 from mrk-andreev/SPARK-49549. Authored-by: Mark Andreev <mark.andreev@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

github-actions bot added the SQL label Sep 27, 2024

mrk-andreev force-pushed the SPARK-49549 branch from 6de8c67 to 9c45cf9 Compare September 28, 2024 16:03

MaxGekk requested changes Oct 2, 2024

View reviewed changes

mrk-andreev force-pushed the SPARK-49549 branch 2 times, most recently from 513c953 to 943845a Compare October 8, 2024 20:30

MaxGekk requested changes Oct 9, 2024

View reviewed changes

mrk-andreev force-pushed the SPARK-49549 branch from 943845a to 9b10fe3 Compare October 9, 2024 09:43

MaxGekk reviewed Oct 9, 2024

View reviewed changes

mrk-andreev force-pushed the SPARK-49549 branch 2 times, most recently from 4ff4cf7 to 186c5bc Compare October 9, 2024 11:36

[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERRO…

bc1d202

…R_TEMP_3055, 3146

mrk-andreev force-pushed the SPARK-49549 branch from 186c5bc to bc1d202 Compare October 9, 2024 15:27

MaxGekk approved these changes Oct 9, 2024

View reviewed changes

MaxGekk closed this in f0498f0 Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERROR_TEMP_3055, 3146 #48288

[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERROR_TEMP_3055, 3146 #48288

mrk-andreev commented Sep 27, 2024 •

edited by MaxGekk

Loading

MaxGekk Oct 2, 2024

mrk-andreev Oct 8, 2024

MaxGekk Oct 2, 2024

mrk-andreev Oct 8, 2024

MaxGekk Oct 2, 2024

mrk-andreev Oct 8, 2024

MaxGekk Oct 2, 2024

mrk-andreev Oct 8, 2024

MaxGekk Oct 9, 2024

mrk-andreev Oct 9, 2024

MaxGekk left a comment

MaxGekk Oct 9, 2024

mrk-andreev Oct 9, 2024

MaxGekk commented Oct 9, 2024

MaxGekk commented Oct 9, 2024

mrk-andreev commented Oct 9, 2024 •

edited

Loading

MaxGekk commented Oct 10, 2024

		"SCALAR_FUNCTION_NOT_COMPATIBLE",
		Map.of("scalarFunc", QuotingUtils.quoteIdentifier(name()))

[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERROR_TEMP_3055, 3146 #48288

[SPARK-49549][SQL] Assign a name to the error conditions _LEGACY_ERROR_TEMP_3055, 3146 #48288

Conversation

mrk-andreev commented Sep 27, 2024 • edited by MaxGekk 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Oct 9, 2024

MaxGekk commented Oct 9, 2024

mrk-andreev commented Oct 9, 2024 • edited Loading

MaxGekk commented Oct 10, 2024

mrk-andreev commented Sep 27, 2024 •

edited by MaxGekk

Loading

mrk-andreev commented Oct 9, 2024 •

edited

Loading