Skip to content
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

Closed

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Dec 13, 2023

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:

  • list tables
  • namespace exists
  • list namespaces

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:

$ build/sbt "test:testOnly *JDBCV2Suite"
$ build/sbt "test:testOnly *JDBCTableCatalogSuite"

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Dec 13, 2023
@MaxGekk MaxGekk changed the title [WIP][SQL] Classify exceptions in the JDBC table catalog [WIP][SPARK-46393][SQL] Classify exceptions in the JDBC table catalog Dec 13, 2023
@MaxGekk MaxGekk marked this pull request as ready for review December 13, 2023 19:51
@MaxGekk MaxGekk changed the title [WIP][SPARK-46393][SQL] Classify exceptions in the JDBC table catalog [SPARK-46393][SQL] Classify exceptions in the JDBC table catalog Dec 13, 2023
@srielau
Copy link
Contributor

srielau commented Dec 13, 2023

@MaxGekk
Is this just doing:
/**

  • Gets a dialect exception, classifies it and wraps it by AnalysisException.
  • @param message The error message to be placed to the returned exception.
  • @param e The dialect specific exception.
  • @return AnalysisException or its sub-class.
    */
    def classifyException(message: String, e: Throwable): AnalysisException = {
    new AnalysisException(
    errorClass = "_LEGACY_ERROR_TEMP_3064",
    messageParameters = Map("msg" -> message),
    cause = Some(e))
    }

Shouldn't we raise proper error classes instead of hiding behind "message"

@cloud-fan
Copy link
Contributor

@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.

@srielau
Copy link
Contributor

srielau commented Dec 14, 2023

@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.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 14, 2023

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.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 14, 2023

Merging to master. Thank you, all for review.

@MaxGekk MaxGekk closed this in f1e5a13 Dec 14, 2023
@srielau
Copy link
Contributor

srielau commented Dec 14, 2023

Opened: https://issues.apache.org/jira/browse/SPARK-46410 as follow up.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 15, 2023

@srielau @cloud-fan The small PR with assigned names is ready: #44358

yaooqinn pushed a commit that referenced this pull request Jun 7, 2024
…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 <wenchen@databricks.com>
Signed-off-by: Kent Yao <yao@apache.org>
panbingkun pushed a commit to panbingkun/spark that referenced this pull request Jun 7, 2024
…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 <wenchen@databricks.com>
Signed-off-by: Kent Yao <yao@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants