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 2021/09/07 21:16:53 UTC
[GitHub] [spark] karenfeng commented on a change in pull request #33864: [SPARK-36296][SQL] Refactor seventh set of 20 in QueryExecutionErrors to use error classes
karenfeng commented on a change in pull request #33864:
URL: https://github.com/apache/spark/pull/33864#discussion_r703832089
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -3,9 +3,29 @@
"message" : [ "Field name %s is ambiguous and has %s matching fields in the struct." ],
"sqlState" : "42000"
},
+ "CANNOT_GET_JDBC_TYPE" : {
+ "message" : [ "Can't get JDBC type for %s" ],
+ "sqlState" : "42000"
+ },
+ "CANNOT_READ_FOOTER_FOR_FILE" : {
+ "message" : [ "Could not read footer for file: %s" ],
+ "sqlState" : "42000"
+ },
+ "CANNOT_TRANSLATE_NON_NULL_VALUE_FOR_FIELD" : {
+ "message" : [ "Can't translate non-null value for field %s" ],
+ "sqlState" : "0A000"
+ },
"CONCURRENT_QUERY_ERROR" : {
"message" : [ "Another instance of this query was just started by a concurrent session." ]
},
+ "DATA_TYPE_UNSUPPORTED_YET" : {
+ "message" : [ "%s is not supported yet." ],
+ "sqlState" : "0A000"
+ },
+ "DDL_UNSUPPORTED_TEMPORARILY" : {
Review comment:
Same here, but remove "temporarily." We should rename the functions as well to avoid confusion.
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -3,9 +3,29 @@
"message" : [ "Field name %s is ambiguous and has %s matching fields in the struct." ],
"sqlState" : "42000"
},
+ "CANNOT_GET_JDBC_TYPE" : {
+ "message" : [ "Can't get JDBC type for %s" ],
+ "sqlState" : "42000"
+ },
+ "CANNOT_READ_FOOTER_FOR_FILE" : {
+ "message" : [ "Could not read footer for file: %s" ],
+ "sqlState" : "42000"
+ },
+ "CANNOT_TRANSLATE_NON_NULL_VALUE_FOR_FIELD" : {
+ "message" : [ "Can't translate non-null value for field %s" ],
+ "sqlState" : "0A000"
+ },
"CONCURRENT_QUERY_ERROR" : {
"message" : [ "Another instance of this query was just started by a concurrent session." ]
},
+ "DATA_TYPE_UNSUPPORTED_YET" : {
Review comment:
As discussed in the [error message guidelines](https://spark.apache.org/error-message-guidelines.html), we should "Avoid time-based statements, such as promises of future support." Let's remove `yet` from the wording here.
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -54,13 +90,25 @@
"message" : [ "The fraction of sec must be zero. Valid range is [0, 60]." ],
"sqlState" : "22023"
},
+ "INVALID_JDBC_NUMPARTITIONS" : {
Review comment:
Nit: `NUMPARTITIONS` -> `NUM_PARTITIONS`
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -14,13 +34,25 @@
"message" : [ "Found duplicate keys '%s'" ],
"sqlState" : "23000"
},
+ "EMPTY_OPTION" : {
+ "message" : [ "Option `%s` can not be empty." ],
+ "sqlState" : "08006"
+ },
"FAILED_RENAME_PATH" : {
"message" : [ "Failed to rename %s to %s as destination already exists" ],
"sqlState" : "22023"
},
"FAILED_SET_ORIGINAL_PERMISSION_BACK" : {
"message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ]
},
+ "FAILED_TO_MERGE_IN_COMPATIBLE_SCHEMAS" : {
+ "message" : [ "Failed to merge incompatible schemas %s and %s" ],
+ "sqlState" : "42000"
+ },
+ "FOUND_DUPLICATE_FIELD_IN_CASE_INSENSITIVE_MODE" : {
+ "message" : [ "%s" ],
Review comment:
We should put the base message in here and provide only the parameters. In this case, we could make the message something like `Found duplicate field(s) %s: %s in case-insensitive mode` and pass in `requiredFieldName` and `matchedOrcFields`.
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -14,13 +34,25 @@
"message" : [ "Found duplicate keys '%s'" ],
"sqlState" : "23000"
},
+ "EMPTY_OPTION" : {
+ "message" : [ "Option `%s` can not be empty." ],
+ "sqlState" : "08006"
+ },
"FAILED_RENAME_PATH" : {
"message" : [ "Failed to rename %s to %s as destination already exists" ],
"sqlState" : "22023"
},
"FAILED_SET_ORIGINAL_PERMISSION_BACK" : {
"message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ]
},
+ "FAILED_TO_MERGE_IN_COMPATIBLE_SCHEMAS" : {
Review comment:
Nit: `IN_COMPATIBLE` -> `INCOMPATIBLE`
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -96,10 +151,22 @@
"message" : [ "Unrecognized SQL type %s" ],
"sqlState" : "42000"
},
+ "UNSUPPORTED_ARRAY_ELEMENT_TYPE_BASED_ON_BINARY" : {
+ "message" : [ "Unsupported array element type %s based on binary" ],
+ "sqlState" : "0A000"
+ },
+ "UNSUPPORTED_JDBC_TYPE" : {
+ "message" : [ "Unsupported type %s" ],
+ "sqlState" : "0A000"
+ },
"UNSUPPORTED_LITERAL_TYPE" : {
"message" : [ "Unsupported literal type %s %s" ],
"sqlState" : "0A000"
},
+ "UNSUPPORTED_OPERATION_FOR_DATA_TYPE" : {
+ "message" : [ "DataType: %s" ],
+ "sqlState" : "42000"
Review comment:
Would 0A000 be a better fit here?
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -54,13 +90,25 @@
"message" : [ "The fraction of sec must be zero. Valid range is [0, 60]." ],
"sqlState" : "22023"
},
+ "INVALID_JDBC_NUMPARTITIONS" : {
+ "message" : [ "Invalid value `%s` for parameter `%s` in table writing via JDBC. The minimum value is 1." ],
+ "sqlState" : "42000"
+ },
+ "INVALID_JDBC_TXN_ISOLATION_LEVEL" : {
Review comment:
As the error-class is user-facing, we should avoid using abbreviations. How about `TRANSACTION` instead of `TXN`?
--
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