[SPARK-36303][SQL] Refactor fourthteenth set of 20 query execution errors to use error classes #33878
Conversation
|
Can one of the admins verify this patch? |
|
cc @karenfeng FYI |
There was a problem hiding this comment.
Thanks for working on this @dgd-contributor! I left some comments.
...atalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogNotFoundException.scala
Outdated
Show resolved
Hide resolved
|
Thanks for your comments @karenfeng , can you review it again? |
There was a problem hiding this comment.
Thanks for working on this @dgd-contributor! I left some comments; please tag me once you get a chance to address them.
| new SparkNoSuchElementException( | ||
| errorClass = "NO_SUCH_ELEMENT_EXCEPTION", | ||
| messageParameters = Array.empty | ||
| ) | ||
| } | ||
|
|
||
| def noSuchElementExceptionError(key: String): Throwable = { | ||
| new NoSuchElementException(key) |
There was a problem hiding this comment.
This should share an error class with the exception above.
| @@ -77,6 +144,9 @@ | |||
| "message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ], | |||
| "sqlState" : "42000" | |||
| }, | |||
| "NO_SUCH_ELEMENT_EXCEPTION" : { | |||
There was a problem hiding this comment.
We should:
- Remove the word
EXCEPTIONfrom the error class. - Update the error message to better reflect what type of error this is, maybe with
No such element %s?
| @@ -21,6 +67,15 @@ | |||
| "FAILED_SET_ORIGINAL_PERMISSION_BACK" : { | |||
| "message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ] | |||
| }, | |||
| "FAILED_TO_CALL_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CONSTRUCTOR" : { | |||
There was a problem hiding this comment.
FAILED_TO_CALL_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CONSTRUCTOR -> FAILED_TO_CALL_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CATALOG
| "FAILED_TO_CALL_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CONSTRUCTOR" : { | ||
| "message" : [ "Failed to call public no-arg constructor for catalog '%s': %s" ] | ||
| }, | ||
| "FAILED_TO_FIND_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CONSTRUCTOR" : { |
There was a problem hiding this comment.
FAILED_TO_FIND_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CONSTRUCTOR -> FAILED_TO_FIND_PUBLIC_NO_ARG_CONSTRUCTOR_FOR_CATALOG
| "Cannot set timeout timestamp without enabling event time timeout in " + | ||
| "[map|flatMapGroupsWithState") | ||
| new SparkUnsupportedOperationException( | ||
| errorClass = "CANNOT_GET_EVENT_TIME_WATERMARK", |
There was a problem hiding this comment.
Is this the right error class?
| "sqlState" : "42000" | ||
| }, | ||
| "CANNOT_SET_TIMEOUT_TIMESTAMP" : { | ||
| "message" : [ "Cannot set timeout timestamp without enabling event time timeout in [map|flatMapGroupsWithState" ], |
There was a problem hiding this comment.
[map|flatMap -> [map|flatMap]
| }, | ||
| "CANNOT_CLONE_OR_COPY_READ_ONLY_SQL_CONF" : { | ||
| "message" : [ "Cannot clone/copy ReadOnlySQLConf." ], | ||
| "sqlState" : "42000" |
There was a problem hiding this comment.
0A000 may be a better fit here
| }, | ||
| "CANNOT_EXECUTE_STREAMING_RELATION_EXEC" : { | ||
| "message" : [ "StreamingRelationExec cannot be executed" ], | ||
| "sqlState" : "42000" |
There was a problem hiding this comment.
This is an internal error; we should remove the SQLSTATE.
| }, | ||
| "CANNOT_MUTATE_READ_ONLY_SQL_CONF" : { | ||
| "message" : [ "Cannot mutate ReadOnlySQLConf." ], | ||
| "sqlState" : "42000" |
There was a problem hiding this comment.
0A000 feels like a better fit for this
| }, | ||
| "CATALOG_PLUGIN_CLASS_NOT_FOUND" : { | ||
| "message" : [ "Catalog '%s' plugin class not found: spark.sql.catalog.%s is not defined" ], | ||
| "sqlState" : "42000" |
There was a problem hiding this comment.
42000 doesn't seem like the right SQLSTATE here. Maybe 3D000?
…ception in RuntimeConfig.scala
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Why are the changes needed?
Adds error classes to some of the exceptions in QueryExecutionErrors.Does this PR introduce any user-facing change?
Improves auditing for developers and adds useful fields for users (error class and SQLSTATE).How was this patch tested?
Existing tests