-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-46393][SQL] Classify exceptions in the JDBC table catalog #44335
Conversation
@MaxGekk
Shouldn't we raise proper error classes instead of hiding behind "message" |
@srielau This seems like a lot of work as we need to understand different errors from different databases. Shall we have a general error class name as the fallback? Each JDBC dialect can override this method and provide more concrete error classes. |
I count 8 invocations of classifyException() in this file. Each invocation adds better information using English. Why can't these be error classes? At a minimum we could give: _LEGACY_ERROR_TEMP_3064 a proper name and each of these invocations picks a subclass. |
There are no tests at all for the cases where JDBC op fails. I believe we should go by the regular way: write tests, update docs, assign names, improve error messages, and so. Let me merge this PR since it fixes obvious holes in the implementation. |
Merging to master. Thank you, all for review. |
Opened: https://issues.apache.org/jira/browse/SPARK-46410 as follow up. |
@srielau @cloud-fan The small PR with assigned names is ready: #44358 |
…loadTable ### What changes were proposed in this pull request? This is a followup of #44335 , which missed to handle `loadTable` ### Why are the changes needed? better error message ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing test ### Was this patch authored or co-authored using generative AI tooling? no Closes #46905 from cloud-fan/jdbc. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Kent Yao <[email protected]>
…loadTable ### What changes were proposed in this pull request? This is a followup of apache#44335 , which missed to handle `loadTable` ### Why are the changes needed? better error message ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#46905 from cloud-fan/jdbc. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Kent Yao <[email protected]>
What changes were proposed in this pull request?
In the PR, I propose to handle exceptions from JDBC drivers in the JDBC table catalog, classify them and converted to appropriate Spark exception w/ an error class. This PR covers the following functions where such errors haven't been classified yet:
Why are the changes needed?
To unify Spark exceptions, and migrate onto new error framework.
Does this PR introduce any user-facing change?
Yes, if user code expects that Spark SQL bypass Java exceptions from JDBC drivers.
How was this patch tested?
By existing test suites:
Was this patch authored or co-authored using generative AI tooling?
No.