You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "miland-db (via GitHub)" <gi...@apache.org> on 2024/03/07 17:02:15 UTC
[PR] Miland db/miland legacy error class [spark]
miland-db opened a new pull request, #45423:
URL: https://github.com/apache/spark/pull/45423
### What changes were proposed in this pull request?
In the PR, I propose to assign the proper names to the legacy error classes _LEGACY_ERROR_TEMP_324[7-9], and modify tests in testing suites to reflect these changes and use checkError() function. Also this PR improves the error messages.
### Why are the changes needed?
Proper name improves user experience w/ Spark SQL.
### Does this PR introduce _any_ user-facing change?
Yes, the PR changes an user-facing error message.
### How was this patch tested?
Tests already exist.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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
Re: [PR] Miland db/miland legacy error class [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1984835207
See also https://spark.apache.org/contributing.html
--
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
Re: [PR] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1988588006
@miland-db Congratulations with your first contribution to Apache Spark!
--
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
Re: [PR] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45423: [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9]
URL: https://github.com/apache/spark/pull/45423
--
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
Re: [PR] [WIP][SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45423:
URL: https://github.com/apache/spark/pull/45423#discussion_r1517818356
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -7822,20 +7822,34 @@
"Failed to parse a value for data type <dataType>."
]
},
- "_LEGACY_ERROR_TEMP_3247" : {
+ "INVALID_DELIMITER_VALUE" : {
"message" : [
- "Delimiter cannot be empty string"
- ]
- },
- "_LEGACY_ERROR_TEMP_3248" : {
- "message" : [
- "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
- ]
+ "Invalid value for delimiter"
+ ],
+ "subClass" : {
+ "EMPTY_STRING": {
+ "message" : [
+ "Delimiter cannot be empty string"
Review Comment:
```suggestion
"Delimiter cannot be empty string."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -7822,20 +7822,34 @@
"Failed to parse a value for data type <dataType>."
]
},
- "_LEGACY_ERROR_TEMP_3247" : {
+ "INVALID_DELIMITER_VALUE" : {
"message" : [
- "Delimiter cannot be empty string"
- ]
- },
- "_LEGACY_ERROR_TEMP_3248" : {
- "message" : [
- "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
- ]
+ "Invalid value for delimiter"
+ ],
+ "subClass" : {
+ "EMPTY_STRING": {
+ "message" : [
+ "Delimiter cannot be empty string"
+ ]
+ },
+ "SINGLE_BACKSLASH": {
+ "message" : [
+ "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
+ ]
+ }
+ }
},
- "_LEGACY_ERROR_TEMP_3249" : {
+ "JSON_CONVERSION_ERROR" : {
Review Comment:
Let's assign more specific name to it: `FAILED_ROW_TO_JSON`
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -7822,20 +7822,34 @@
"Failed to parse a value for data type <dataType>."
]
},
- "_LEGACY_ERROR_TEMP_3247" : {
+ "INVALID_DELIMITER_VALUE" : {
"message" : [
- "Delimiter cannot be empty string"
- ]
- },
- "_LEGACY_ERROR_TEMP_3248" : {
- "message" : [
- "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
- ]
+ "Invalid value for delimiter"
+ ],
+ "subClass" : {
+ "EMPTY_STRING": {
+ "message" : [
+ "Delimiter cannot be empty string"
+ ]
+ },
+ "SINGLE_BACKSLASH": {
+ "message" : [
+ "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
+ ]
+ }
+ }
},
- "_LEGACY_ERROR_TEMP_3249" : {
+ "JSON_CONVERSION_ERROR" : {
"message" : [
- "Failed to convert value <value> (class of <valueClass>}) with the type of <dataType> to JSON."
- ]
+ "Invalid JSON conversion."
+ ],
+ "subClass" : {
+ "UNSUPPORTED_VALUE_TYPE" : {
+ "message" : [
+ "Failed to convert value <value> (class of <valueClass>}) with the type of <dataType> to JSON."
Review Comment:
```suggestion
"Failed to convert the row value <value> of the class <class> to the target SQL type <sqlType> in the JSON format."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -7822,20 +7822,34 @@
"Failed to parse a value for data type <dataType>."
]
},
- "_LEGACY_ERROR_TEMP_3247" : {
+ "INVALID_DELIMITER_VALUE" : {
"message" : [
- "Delimiter cannot be empty string"
- ]
- },
- "_LEGACY_ERROR_TEMP_3248" : {
- "message" : [
- "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
- ]
+ "Invalid value for delimiter"
Review Comment:
```suggestion
"Invalid value for delimiter."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -7822,20 +7822,34 @@
"Failed to parse a value for data type <dataType>."
]
},
- "_LEGACY_ERROR_TEMP_3247" : {
+ "INVALID_DELIMITER_VALUE" : {
"message" : [
- "Delimiter cannot be empty string"
- ]
- },
- "_LEGACY_ERROR_TEMP_3248" : {
- "message" : [
- "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
- ]
+ "Invalid value for delimiter"
+ ],
+ "subClass" : {
+ "EMPTY_STRING": {
+ "message" : [
+ "Delimiter cannot be empty string"
+ ]
+ },
+ "SINGLE_BACKSLASH": {
+ "message" : [
+ "Single backslash is prohibited. It has special meaning as beginning of an escape sequence. To get the backslash character, pass a string with two backslashes as the delimiter."
+ ]
+ }
+ }
},
- "_LEGACY_ERROR_TEMP_3249" : {
+ "JSON_CONVERSION_ERROR" : {
"message" : [
- "Failed to convert value <value> (class of <valueClass>}) with the type of <dataType> to JSON."
- ]
+ "Invalid JSON conversion."
+ ],
+ "subClass" : {
+ "UNSUPPORTED_VALUE_TYPE" : {
Review Comment:
The sub-class is too much for the case, I believe. Let's inline the error message to parent's message.
--
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
Re: [PR] Miland db/miland legacy error class [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1984834978
Mind filing a JIRA and linking it to the PR title 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
Re: [PR] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1988766106
Thank you! @MaxGekk and thank you @HyukjinKwon for the comments
--
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
Re: [PR] Miland db/miland legacy error class [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db closed pull request #45423: Miland db/miland legacy error class
URL: https://github.com/apache/spark/pull/45423
--
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
Re: [PR] [WIP][SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45423:
URL: https://github.com/apache/spark/pull/45423#discussion_r1517835712
##########
sql/api/src/main/scala/org/apache/spark/sql/Row.scala:
##########
@@ -611,7 +611,7 @@ trait Row extends Serializable {
case (v: Any, udt: UserDefinedType[Any @unchecked]) =>
toJson(UDTUtils.toRow(v, udt), udt.sqlType)
case _ => throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3249",
+ errorClass = "JSON_CONVERSION_ERROR.UNSUPPORTED_VALUE_TYPE",
Review Comment:
Please, quote `dataType` by `toSQLType`, and `value` by `toSQLValue`
--
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
Re: [PR] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1988584943
+1, LGTM. Merging to master.
Thank you, @miland-db and @HyukjinKwon for review.
--
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
Re: [PR] [WIP][SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on PR #45423:
URL: https://github.com/apache/spark/pull/45423#issuecomment-1985380767
I corrected the title, and I am attaching the JIRA link: https://issues.apache.org/jira/browse/SPARK-47255
--
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