You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/06 15:41:06 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #38940: [SPARK-41409][CORE][SQL] Reuse `WRONG_NUM_ARGS` instead of `_LEGACY_ERROR_TEMP_1043`
LuciferYang opened a new pull request, #38940:
URL: https://github.com/apache/spark/pull/38940
### What changes were proposed in this pull request?
This pr aims to reuse error class `WRONG_NUM_ARGS` instead of `_LEGACY_ERROR_TEMP_1043`.
### 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?
Pass Github Actions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1042955098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -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",
Review Comment:
This may be triggered by the user action, such as `SELECT CAST(1)` in `UDFSuite.scala`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [WIP][SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1041802953
##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}
test("SPARK-28521 error message for CAST(parameter types contains DataType)") {
- 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)")
Review Comment:
Do you have any suggestions on the calculation way of `validParametersCount` @MaxGekk ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [WIP][SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1041778431
##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}
test("SPARK-28521 error message for CAST(parameter types contains DataType)") {
- 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)")
Review Comment:
https://github.com/apache/spark/blob/37453ad5c85122d11b436813b4f5eddf615a6cf8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L137-L144
In this scenario, the `validParametersCount` is also empty, so `WRONG_NUM_ARGS ` cannot be reused now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1045475072
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -714,6 +714,11 @@
],
"sqlState" : "22023"
},
+ "INVALID_FUNCTION_ARGS" : {
Review Comment:
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>.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}
test("SPARK-28521 error message for CAST(parameter types contains DataType)") {
Review Comment:
Let's move this test to `QueryCompilationsErrorsSuite`
##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}
test("SPARK-28521 error message for CAST(parameter types contains DataType)") {
- 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)")
Review Comment:
The code above is not correct actually in some cases. One more example is `TimestampAdd`:
https://github.com/apache/spark/blob/a3a755d36136295473a4873a6df33c295c29213e/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala#L3156-L3159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1042879224
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -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",
Review Comment:
@MaxGekk This should be an internal exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1042955098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -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",
Review Comment:
This may be triggered by the user action, such as `SELECT CAST(1)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
URL: https://github.com/apache/spark/pull/38940
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38940:
URL: https://github.com/apache/spark/pull/38940#issuecomment-1350470301
Thanks @MaxGekk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38940:
URL: https://github.com/apache/spark/pull/38940#issuecomment-1350415022
+1, LGTM. Merging to master.
Thank you, @LuciferYang.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1046766089
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1526,8 +1526,20 @@
},
"WRONG_NUM_ARGS" : {
"message" : [
- "The <functionName> requires <expectedNum> parameters but the actual number is <actualNum>."
- ]
+ "Invalid number of arguments for the function <functionName>."
+ ],
+ "subClass" : {
+ "WITHOUT_SUGGESTION" : {
+ "message" : [
+ "Please ref to 'https://spark.apache.org/docs/latest/sql-ref-functions.html' for fix."
+ ]
+ },
+ "WITH_SUGGESTION" : {
+ "message" : [
+ "The function requires <expectedNum> parameters but the actual number is <actualNum>."
Review Comment:
The sentence still looks like a description of an issue but not a suggestion.
```suggestion
"Consider to change the number of arguments because the function requires <expectedNum> parameters but the actual number is <actualNum>."
```
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1526,8 +1526,20 @@
},
"WRONG_NUM_ARGS" : {
"message" : [
- "The <functionName> requires <expectedNum> parameters but the actual number is <actualNum>."
- ]
+ "Invalid number of arguments for the function <functionName>."
+ ],
+ "subClass" : {
+ "WITHOUT_SUGGESTION" : {
+ "message" : [
+ "Please ref to 'https://spark.apache.org/docs/latest/sql-ref-functions.html' for fix."
Review Comment:
```suggestion
"Please, refer to 'https://spark.apache.org/docs/latest/sql-ref-functions.html' for a fix."
```
##########
sql/core/src/test/resources/sql-tests/results/table-valued-functions.sql.out:
##########
@@ -83,7 +83,7 @@ org.apache.spark.sql.AnalysisException
"errorClass" : "_LEGACY_ERROR_TEMP_1179",
"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.",
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38940:
URL: https://github.com/apache/spark/pull/38940#issuecomment-1350253955
GA passed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [WIP][SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `INVALID_FUNCTION_ARGS`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1041802953
##########
sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala:
##########
@@ -638,10 +638,16 @@ class UDFSuite extends QueryTest with SharedSparkSession {
}
test("SPARK-28521 error message for CAST(parameter types contains DataType)") {
- 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)")
Review Comment:
Do you have any suggestions on the calculation way of `validParametersCount `?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38940: [SPARK-41409][CORE][SQL] Rename `_LEGACY_ERROR_TEMP_1043` to `WRONG_NUM_ARGS.WITHOUT_SUGGESTION`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38940:
URL: https://github.com/apache/spark/pull/38940#discussion_r1047007922
##########
sql/core/src/test/resources/sql-tests/results/table-valued-functions.sql.out:
##########
@@ -83,7 +83,7 @@ org.apache.spark.sql.AnalysisException
"errorClass" : "_LEGACY_ERROR_TEMP_1179",
"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.",
Review Comment:
[SPARK-41508](https://issues.apache.org/jira/browse/SPARK-41508)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org