[SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION` #38940

LuciferYang · 2022-12-06T15:41:03Z

What changes were proposed in this pull request?

This pr introduces sub-classes of WRONG_NUM_ARGS:

WITHOUT_SUGGESTION
WITH_SUGGESTION

then replace existing WRONG_NUM_ARGS to WRONG_NUM_ARGS.WITH_SUGGESTION and rename error class _LEGACY_ERROR_TEMP_1043 to WRONG_NUM_ARGS.WITHOUT_SUGGESTION

Why are the changes needed?

Proper names of error classes to improve user experience with Spark SQL.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add new test case

LuciferYang · 2022-12-07T05:37:05Z

sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala

-    assert(e.getMessage.contains("Invalid arguments for function cast"))
+    checkError(
+      exception = intercept[AnalysisException] {
+        sql("SELECT CAST(1)")


spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

Lines 137 to 144 in 37453ad

// Otherwise, find a constructor method that matches the number of arguments, and use that.

val params = Seq.fill(expressions.size)(classOf[Expression])

val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse {

val validParametersCount = constructors

.filter(_.getParameterTypes.forall(_ == classOf[Expression]))

.map(_.getParameterCount).distinct.sorted

throw QueryCompilationErrors.invalidFunctionArgumentNumberError(

validParametersCount, name, params.length)

In this scenario, the validParametersCount is also empty, so WRONG_NUM_ARGS cannot be reused now

Do you have any suggestions on the calculation way of validParametersCount @MaxGekk ?

The code above is not correct actually in some cases. One more example is TimestampAdd:

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

Lines 3156 to 3159 in a3a755d

case class TimestampAdd(

unit: String,

quantity: Expression,

timestamp: Expression,

LuciferYang · 2022-12-08T03:08:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

      new AnalysisException(
-        errorClass = "_LEGACY_ERROR_TEMP_1043",
-        messageParameters = Map("name" -> name))
+        errorClass = "INVALID_FUNCTION_ARGS",


@MaxGekk This should be an internal exception?

This may be triggered by the user action, such as SELECT CAST(1) in UDFSuite.scala

MaxGekk · 2022-12-10T08:36:53Z

sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala

    assert(spark.range(2).select(nonDeterministicJavaUDF()).distinct().count() == 2)
  }

  test("SPARK-28521 error message for CAST(parameter types contains DataType)") {


Let's move this test to QueryCompilationsErrorsSuite

MaxGekk · 2022-12-10T16:58:30Z

sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala

-    assert(e.getMessage.contains("Invalid arguments for function cast"))
+    checkError(
+      exception = intercept[AnalysisException] {
+        sql("SELECT CAST(1)")


The code above is not correct actually in some cases. One more example is TimestampAdd:

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

Lines 3156 to 3159 in a3a755d

case class TimestampAdd(

unit: String,

quantity: Expression,

timestamp: Expression,

MaxGekk · 2022-12-12T07:22:57Z

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

    ],
    "sqlState" : "22023"
  },
+  "INVALID_FUNCTION_ARGS" : {


How about to introduce sub-classes of WRONG_NUM_ARGS:

WITHOUT_SUGGESTION

WITH_SUGGESTION

And declare in the common message template that

Invalid number of arguments for the function <funcName>.

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

MaxGekk · 2022-12-13T07:57:41Z

sql/core/src/test/resources/sql-tests/results/table-valued-functions.sql.out

  "messageParameters" : {
    "arguments" : "integer, integer, integer, integer, integer",
-    "details" : "[WRONG_NUM_ARGS] The `range` requires [1, 2, 3, 4] parameters but the actual number is 5.",
+    "details" : "[WRONG_NUM_ARGS.WITH_SUGGESTION] Invalid number of arguments for the function `range`. The function requires [1, 2, 3, 4] parameters but the actual number is 5.",


I think we should bypass the error. Could you open an separate PR and check if the exception is SparkThrowable then re-throw it, please.

SPARK-41508

Co-authored-by: Maxim Gekk <[email protected]>

MaxGekk

Waiting for CI.

LuciferYang · 2022-12-14T02:10:00Z

GA passed

MaxGekk · 2022-12-14T05:22:05Z

+1, LGTM. Merging to master.
Thank you, @LuciferYang.

LuciferYang · 2022-12-14T06:24:18Z

Thanks @MaxGekk

…UM_ARGS.WITHOUT_SUGGESTION` ### What changes were proposed in this pull request? This pr introduces sub-classes of `WRONG_NUM_ARGS`: - WITHOUT_SUGGESTION - WITH_SUGGESTION then replace existing `WRONG_NUM_ARGS` to `WRONG_NUM_ARGS.WITH_SUGGESTION` and rename error class `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION` ### Why are the changes needed? Proper names of error classes to improve user experience with Spark SQL. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add new test case Closes apache#38940 from LuciferYang/legacy-1043. Lead-authored-by: yangjie01 <[email protected]> Co-authored-by: YangJie <[email protected]> Signed-off-by: Max Gekk <[email protected]>

LuciferYang added 2 commits December 6, 2022 23:31

replace

a131087

upgrade golden files

a8f8246

github-actions bot added CORE SQL labels Dec 6, 2022

LuciferYang marked this pull request as draft December 6, 2022 15:41

LuciferYang added 3 commits December 6, 2022 23:43

change case

e8b70cd

update golden files

f3534c8

update golden files

4984065

LuciferYang changed the title ~~[SPARK-41409][CORE][SQL] Reuse WRONG_NUM_ARGS instead of _LEGACY_ERROR_TEMP_1043~~ [WIP][SPARK-41409][CORE][SQL] Reuse WRONG_NUM_ARGS instead of _LEGACY_ERROR_TEMP_1043 Dec 7, 2022

LuciferYang added 2 commits December 7, 2022 13:29

rename

381b886

revert sql files

53abc82

LuciferYang changed the title ~~[WIP][SPARK-41409][CORE][SQL] Reuse WRONG_NUM_ARGS instead of _LEGACY_ERROR_TEMP_1043~~ [WIP][SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to INVALID_FUNCTION_ARGS Dec 7, 2022

LuciferYang commented Dec 7, 2022

View reviewed changes

revert

fe811b8

LuciferYang changed the title ~~[WIP][SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to INVALID_FUNCTION_ARGS~~ [SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to INVALID_FUNCTION_ARGS Dec 7, 2022

LuciferYang mentioned this pull request Dec 8, 2022

[SPARK-41435][SQL] Change to call invalidFunctionArgumentsError for curdate() when expressions is not empty #38960

Closed

LuciferYang commented Dec 8, 2022

View reviewed changes

MaxGekk reviewed Dec 12, 2022

View reviewed changes

LuciferYang added 4 commits December 13, 2022 12:12

fix

0cfc968

remove INVALID_FUNCTION_ARGS

4a19124

update golden files

fbc7809

add mesg

a6302e6

LuciferYang changed the title ~~[SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to INVALID_FUNCTION_ARGS~~ [SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to WRONG_NUM_ARGS.WITHOUT_SUGGESTION Dec 13, 2022

fix test

57bf670

LuciferYang marked this pull request as ready for review December 13, 2022 06:12

MaxGekk reviewed Dec 13, 2022

View reviewed changes

LuciferYang and others added 4 commits December 13, 2022 19:10

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

9967415

Co-authored-by: Maxim Gekk <[email protected]>

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

e13a7e0

Co-authored-by: Maxim Gekk <[email protected]>

Merge branch 'upmaster' into legacy-1043

e258ce7

update golden files

1cacfc1

fix test

2a8dd0e

MaxGekk approved these changes Dec 13, 2022

View reviewed changes

MaxGekk closed this in 4e8980e Dec 14, 2022

	// Otherwise, find a constructor method that matches the number of arguments, and use that.
	val params = Seq.fill(expressions.size)(classOf[Expression])
	val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse {
	val validParametersCount = constructors
	.filter(_.getParameterTypes.forall(_ == classOf[Expression]))
	.map(_.getParameterCount).distinct.sorted
	throw QueryCompilationErrors.invalidFunctionArgumentNumberError(
	validParametersCount, name, params.length)

	case class TimestampAdd(
	unit: String,
	quantity: Expression,
	timestamp: Expression,

[SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to WRONG_NUM_ARGS.WITHOUT_SUGGESTION #38940

[SPARK-41409][CORE][SQL] Rename _LEGACY_ERROR_TEMP_1043 to WRONG_NUM_ARGS.WITHOUT_SUGGESTION #38940

Uh oh!

Conversation

LuciferYang commented Dec 6, 2022 • edited by MaxGekk 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!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Dec 7, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Dec 8, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Dec 14, 2022

Uh oh!

MaxGekk commented Dec 14, 2022

Uh oh!

LuciferYang commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION` #38940

[SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION` #38940

LuciferYang commented Dec 6, 2022 •

edited by MaxGekk

Loading

LuciferYang Dec 7, 2022 •

edited

Loading

LuciferYang Dec 8, 2022 •

edited

Loading