-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-36107][SQL] Refactor first set of 20 query execution errors to use error classes #33538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Can one of the admins verify this patch? |
245b6a3 to
6ec85b9
Compare
|
@karenfeng FYI |
karenfeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! We'll need to create some new SparkThrowable implementations; if we simply throw the existing exception types, the error class and SQLSTATE are not propagated through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a new exception type that mixes UnsupportedOperationException with SparkThrowable? It'd look like SparkArithmeticException. Then we can catch these exceptions and use their error classes/SQLSTATEs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I'll do it. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karenfeng I also add SparkUnsupportedOperationException, SparkIllegalStateException,SparkNumberFormatException,SparkIllegalArgumentException,SparkArrayIndexOutOfBoundsException,SparkNoSuchElementException that with SparkThrowable.
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
Outdated
Show resolved
Hide resolved
43d06f0 to
8f1508c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that HY represents CLI-specific conditions. This seems to be triggered by overflows; maybe 22023 is a better fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Maybe 22023 is a better fit. The exception is triggered by invalid parameter value. Thank you.
98a1563 to
b44e8fa
Compare
karenfeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! LGTM. @HyukjinKwon, can you also take a look?
98d7635 to
db6d8d3
Compare
1. add SparkUnsupportedOperationException
SparkIllegalStateException
SparkNumberFormatException
SparkIllegalArgumentException
SparkArrayIndexOutOfBoundsException
SparkNoSuchElementException
2. change the sqlstate suitable code and order the error in
error-class.json
3. others
|
@HyukjinKwon Could you take a look? |
…xecutionErrors ### What changes were proposed in this pull request? When we refactor the query execution errors to use error classes in QueryExecutionErrors, we need define some exception that mix SparkThrowable into a base Exception type. according the example [SparkArithmeticException](https://github.com/apache/spark/blob/f90eb6a5db0778fd18b0b544f93eac3103bbf03b/core/src/main/scala/org/apache/spark/SparkException.scala#L75) Add SparkXXXException as follows: - `SparkClassNotFoundException` - `SparkConcurrentModificationException` - `SparkDateTimeException` - `SparkFileAlreadyExistsException` - `SparkFileNotFoundException` - `SparkNoSuchMethodException` - `SparkIndexOutOfBoundsException` - `SparkIOException` - `SparkSecurityException` - `SparkSQLException` - `SparkSQLFeatureNotSupportedException` Refactor some exceptions in QueryExecutionErrors to use error classes and new exception for testing new exception Some added by [PR](#33538) as follows: - `SparkUnsupportedOperationException` - `SparkIllegalStateException` - `SparkNumberFormatException` - `SparkIllegalArgumentException` - `SparkArrayIndexOutOfBoundsException` - `SparkNoSuchElementException` ### Why are the changes needed? [SPARK-36336](https://issues.apache.org/jira/browse/SPARK-36336) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? existed ut test Closes #33573 from Peng-Lei/SPARK-36336. Authored-by: PengLei <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
| }, | ||
| "FAILED_EXECUTE_UDF" : { | ||
| "message" : [ "Failed to execute user defined function (%s: (%s) => %s)" ], | ||
| "sqlState" : "45000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQLSTATE '45000' is not valid. Because it is not included in README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karenfeng I delete it.
|
seems like the tests fail. Can you fix it up please? |
Thank you. Done |
|
Sorry I saw this late. There have been few changes made. @karenfeng mind double checking before we go ahead please? |
karenfeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @Peng-Lei! I left some comments - please tag me once you address them.
|
@karenfeng I've already address the comments. Please check. Thank you. |
karenfeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @Peng-Lei! @HyukjinKwon, can you help take a second look?
|
Merged to master. |
|
@karenfeng @HyukjinKwon Thank you! |
| "message" : [ "Field name %s is ambiguous and has %s matching fields in the struct." ], | ||
| "sqlState" : "42000" | ||
| }, | ||
| "CANNOT_CAST_DATATYPE" : { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas how to trigger this error class? I haven't found any ways how trigger it from user code. WDYT? @cloud-fan @HyukjinKwon
What changes were proposed in this pull request?
Refactor some exceptions in QueryExecutionErrors to use error classes. as follows:
Why are the changes needed?
SPARK-36107
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existed UT Testcase