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.

Sign up for GitHub

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

Jump to bottom

[SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.loadTable and Fix UT #46912

Closed
wants to merge 4 commits into from
Closed

[SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.loadTable and Fix UT #46912

wants to merge 4 commits into from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jun 7, 2024 •
edited
Loading

What changes were proposed in this pull request?

This is a followup of #46905, to fix some UT on GA.

Why are the changes needed?

Fix UT.

Does this PR introduce any user-facing change?

No.,

How was this patch tested?

Manually test.
Pass GA

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

No.

@github-actions github-actions bot added the SQL label Jun 7, 2024
@panbingkun panbingkun mentioned this pull request Jun 7, 2024
Closed
@panbingkun panbingkun marked this pull request as ready for review June 7, 2024 08:18
@panbingkun
Copy link
Contributor Author

cc @cloud-fan @yaooqinn @HyukjinKwon

@yaooqinn
Copy link
Member

yaooqinn commented Jun 7, 2024

Could you cherry-pick 82b4ad2 here to test the CI?

@panbingkun
Copy link
Contributor Author

Could you cherry-pick 82b4ad2 here to test the CI?

Okay.

panbingkun and others added 2 commits June 7, 2024 16:37
@panbingkun
Merge branch 'master' into SPARK-46393_FOLLOWUP
2d136ef
@cloud-fan @panbingkun
[SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.…
60b4228 
…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]>
@panbingkun
Copy link
Contributor Author

Could you cherry-pick 82b4ad2 here to test the CI?

Done.

yaooqinn
yaooqinn approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Pending CIs

@yaooqinn yaooqinn changed the title [SPARK-46393][SQL][TESTS] Fix UT [SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.loadTable and Fix UT Jun 7, 2024
panbingkun
panbingkun commented Jun 7, 2024
View reviewed changes
...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTableCatalog.scala
"url" -> options.getRedactUrl(),
"tableName" -> toSQLId(ident)),
dialect,
description = s"Failed to load table: $ident"
Copy link
Contributor Author

@panbingkun panbingkun Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan @yaooqinn
Also, do we need catalogname as the prefix for the table here?
Although I found this class JDBCTableCatalog.scala, similar ones do not have this prefix attached.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to add catalog info. Let's do it in a separated PR, as not only loadTable needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Let me to do it in a separated PR.

cloud-fan
cloud-fan reviewed Jun 7, 2024
View reviewed changes
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
def checkErrorFailedLoadTable(e: AnalysisException, tbl: String): Unit = {
checkError(
exception = e,
errorClass = "FAILED_JDBC.UNCLASSIFIED",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be LOAD_TABLE?

Copy link
Contributor Author

@panbingkun panbingkun Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spark/sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

Lines 746 to 768 in 87b0f59

def classifyException(
e: Throwable,
errorClass: String,
messageParameters: Map[String, String],
description: String): AnalysisException = {
classifyException(description, e)
}
/**
* 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.
*/
@deprecated("Please override the classifyException method with an error class", "4.0.0")
def classifyException(message: String, e: Throwable): AnalysisException = {
new AnalysisException(
errorClass = "FAILED_JDBC.UNCLASSIFIED",
messageParameters = Map(
"url" -> "jdbc:",
"message" -> message),
cause = Some(e))
}

image
It lost the upstream errorClass in this place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we should correct it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If necessary, I handle it together with the prefix logic above?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 8911d59 Jun 7, 2024
@panbingkun panbingkun mentioned this pull request Jun 11, 2024
Closed
LuciferYang pushed a commit that referenced this pull request Jun 18, 2024
@panbingkun @LuciferYang
[SPARK-48585][SQL] Make built-in JdbcDialect's method `classifyExce…
a3feffd 
…ption` throw out the `original` exception

### What changes were proposed in this pull request?
The pr aims to make `built-in` JdbcDialect's method classifyException throw out the `original` exception.

### Why are the changes needed?
As discussed in #46912 (comment), the following code:
https://github.com/apache/spark/blob/df4156aa3217cf0f58b4c6cbf33c967bb43f7155/sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala#L746-L751
have lost the original cause of the error, let's correct it.

### Does this PR introduce _any_ user-facing change?
Yes, more accurate error conditions for end users.

### How was this patch tested?
- Manually test.
- Update existed UT & Pass GA.

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

Closes #46937 from panbingkun/improve_JDBCTableCatalog.

Authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloud-fan cloud-fan cloud-fan approved these changes

@yaooqinn yaooqinn yaooqinn approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@panbingkun @yaooqinn @cloud-fan