[SPARK-35347][SQL][FOLLOWUP] Throw exception with an explicit exception type when cannot find the method instead of sys.error #32519
Conversation
|
cc @cloud-fan |
I don't think this is true though .. it's just a short cut of |
| val method = MethodUtils.getMatchingAccessibleMethod(cls, functionName, argClasses: _*) | ||
| if (method == null) { | ||
| sys.error(s"Couldn't find $functionName on $cls") | ||
| throw QueryExecutionErrors.methodNotDeclaredError(functionName) |
There was a problem hiding this comment.
This file has one more sys.error?
The change looks fine for consistency.
There was a problem hiding this comment.
Fix together? Or another minor PR as this is a followup only.
There was a problem hiding this comment.
hm. I think either is okay, but how about assigning a new jira number for replacing all the sys.error in this file with QueryExecutionErrors.xxx methods?
Oh, yea, you're right. cc @cloud-fan package object sys {
/** Throw a new RuntimeException with the supplied message.
*
* @return Nothing.
*/
def error(message: String): Nothing = throw new RuntimeException(message)
... |
|
ah, somehow I misremember this. Anyway it's better to throw exception manually with an explicit exception type. |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #138433 has finished for PR 32519 at commit
|
|
BTW, please revise the PR title, @viirya . |
viirya
changed the title
|
Thanks all! I'm going to merge this first. As this is a followup only, I also find other |
### What changes were proposed in this pull request? This patch replaces `sys.err` usages with explicit exception types. ### Why are the changes needed? Motivated by the previous comment #32519 (comment), it sounds better to replace `sys.err` usages with explicit exception type. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes #32535 from viirya/replace-sys-err. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
6 participants
What changes were proposed in this pull request?
A simple follow-up of #32474 to throw exception instead of sys.error.
Why are the changes needed?
An exception only fails the query, instead of sys.error.
Does this PR introduce any user-facing change?
Yes, if
InvokeorStaticInvokecannot find the method, instead of originalsys.errornow we only throw an exception.How was this patch tested?
Existing tests.