[SPARK-38689][SQL] Use error classes in the compilation errors of not allowed DESC PARTITION on views #36163
Conversation
ivoson
changed the title
|
cc @MaxGekk . Please take a loot at this PR. Thanks. |
|
Can one of the admins verify this patch? |
| new AnalysisException(s"DESC PARTITION is not allowed on a view: $table") | ||
| new AnalysisException( | ||
| errorClass = "INVALID_OPERATION_ON_VIEW", | ||
| messageParameters = Array(toSQLValue("DESC PARTITION"), toSQLValue(table))) |
There was a problem hiding this comment.
Precisely speaking, DESC PARTITION is not a value. Actually, it is a part of sql statement. Could you add a method toSQLStmt and invoke it here. We will call it for quoting of sql statements in error messages.
| val e = intercept[AnalysisException]( | ||
| sql(s"DESC TABLE $tempViewName PARTITION (c='Us', d=1)") | ||
| ) | ||
| assert(e.errorClass === Some("INVALID_OPERATION_ON_TEMP_VIEW")) |
There was a problem hiding this comment.
nit: for consistency with other places
| assert(e.errorClass === Some("INVALID_OPERATION_ON_TEMP_VIEW")) | |
| assert(e.getErrorClass === "INVALID_OPERATION_ON_TEMP_VIEW") |
| "INVALID_OPERATION_ON_TEMP_VIEW" : { | ||
| "message" : [ "Operation %s is not allowed on a temporary view: %s" ] | ||
| }, | ||
| "INVALID_OPERATION_ON_VIEW" : { | ||
| "message" : [ "Operation %s is not allowed on a view: %s" ] | ||
| }, |
There was a problem hiding this comment.
Don't think we should introduce two error classes for almost the same. Could you add only one but more generic:
"FORBIDDEN_OPERATION" : {
"message" : [ "The operation %s is not allowed on %s: %s" ]
},
Don't think we should translate values of second arg to local languages: "the temporary view" and "the view:".
There was a problem hiding this comment.
Done. Thanks for the suggestion.
| new AnalysisException( | ||
| errorClass = "FORBIDDEN_OPERATION", | ||
| messageParameters = Array( | ||
| toSQLStmt("DESC PARTITION"), "the view", toSQLValue(table))) |
There was a problem hiding this comment.
table is not a value, please, use toSQLId instead of toSQLValue
| new AnalysisException( | ||
| errorClass = "FORBIDDEN_OPERATION", | ||
| messageParameters = | ||
| Array(toSQLStmt("DESC PARTITION"), "the temporary view", toSQLValue(table))) |
There was a problem hiding this comment.
Please, use toSQLId instead of toSQLValue, see #36210
|
+1, LGTM. All GAs passed: https://github.com/ivoson/spark/actions/runs/2181933167. Merging to master. |
|
Thanks for the review. @MaxGekk |
3 participants
What changes were proposed in this pull request?
Migrate the following errors in QueryCompilationErrors:
Why are the changes needed?
Porting compilation errors of desc partition on views to new error framework.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add new UT.