Skip to content

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

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
LuciferYang wants to merge 18 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a131087
replace
LuciferYang Dec 6, 2022
a8f8246
upgrade golden files
LuciferYang Dec 6, 2022
e8b70cd
change case
LuciferYang Dec 6, 2022
f3534c8
update golden files
LuciferYang Dec 6, 2022
4984065
update golden files
LuciferYang Dec 7, 2022
381b886
rename
LuciferYang Dec 7, 2022
53abc82
revert sql files
LuciferYang Dec 7, 2022
fe811b8
revert
LuciferYang Dec 7, 2022
0cfc968
fix
LuciferYang Dec 13, 2022
4a19124
remove INVALID_FUNCTION_ARGS
LuciferYang Dec 13, 2022
fbc7809
update golden files
LuciferYang Dec 13, 2022
a6302e6
add mesg
LuciferYang Dec 13, 2022
57bf670
fix test
LuciferYang Dec 13, 2022
9967415
Update core/src/main/resources/error/error-classes.json
LuciferYang Dec 13, 2022
e13a7e0
Update core/src/main/resources/error/error-classes.json
LuciferYang Dec 13, 2022
e258ce7
Merge branch 'upmaster' into legacy-1043
LuciferYang Dec 13, 2022
1cacfc1
update golden files
LuciferYang Dec 13, 2022
2a8dd0e
fix test
LuciferYang Dec 13, 2022
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
10 changes: 5 additions & 5 deletions core/src/main/resources/error/error-classes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,11 @@
],
"sqlState" : "22023"
},
"INVALID_FUNCTION_ARGS" : {

@MaxGekk MaxGekk Dec 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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>.

"message" : [
"Invalid arguments for function <name>."
]
},
"INVALID_IDENTIFIER" : {
"message" : [
"The identifier <ident> is invalid. Please, consider quoting it with back-quotes as `<ident>`."
Expand Down Expand Up @@ -2006,11 +2011,6 @@
"Undefined function <name>."
]
},
"_LEGACY_ERROR_TEMP_1043" : {
"message" : [
"Invalid arguments for function <name>."
]
},
"_LEGACY_ERROR_TEMP_1045" : {
"message" : [
"ALTER TABLE SET LOCATION does not support partition for v2 tables."
Expand Down
6 changes: 3 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,10 +649,10 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {

def invalidFunctionArgumentNumberError(
validParametersCount: Seq[Int], name: String, actualNumber: Int): Throwable = {
if (validParametersCount.length == 0) {
if (validParametersCount.isEmpty) {
new AnalysisException(
errorClass = "_LEGACY_ERROR_TEMP_1043",
messageParameters = Map("name" -> name))
errorClass = "INVALID_FUNCTION_ARGS",

@LuciferYang LuciferYang Dec 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@MaxGekk This should be an internal exception?

@LuciferYang LuciferYang Dec 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

messageParameters = Map("name" -> toSQLId(name)))
} else {
val expectedNumberOfParameters = if (validParametersCount.length == 1) {
validParametersCount.head.toString
Expand Down
11 changes: 11 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@ class DateFunctionsSuite extends QueryTest with SharedSparkSession {
sql("""SELECT CURDATE()""").collect().head.getDate(0))
val d4 = DateTimeUtils.currentDate(ZoneId.systemDefault())
assert(d0 <= d1 && d1 <= d2 && d2 <= d3 && d3 <= d4 && d4 - d0 <= 1)

checkError(
exception = intercept[AnalysisException] {
sql("SELECT CURDATE(1)")
},
errorClass = "INVALID_FUNCTION_ARGS",
parameters = Map(
"name" -> "`curdate`"
),
context = ExpectedContext("", "", 7, 16, "CURDATE(1)")
)
}

test("function current_timestamp and now") {
Expand Down
14 changes: 10 additions & 4 deletions sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}

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

@MaxGekk MaxGekk Dec 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's move this test to QueryCompilationsErrorsSuite

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

@LuciferYang LuciferYang Dec 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@LuciferYang LuciferYang Dec 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@MaxGekk MaxGekk Dec 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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,

},
errorClass = "INVALID_FUNCTION_ARGS",
parameters = Map(
"name" -> "`cast`"
),
context = ExpectedContext("", "", 7, 13, "CAST(1)")
)
}

test("only one case class parameter") {
Expand Down