[SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.loadTable and Fix UT by panbingkun · Pull Request #46912 · apache/spark

panbingkun · 2024-06-07T08:10:26Z

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.

panbingkun · 2024-06-07T08:25:00Z

cc @cloud-fan @yaooqinn @HyukjinKwon

yaooqinn · 2024-06-07T08:34:48Z

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

panbingkun · 2024-06-07T08:35:28Z

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

Okay.

…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>

panbingkun · 2024-06-07T08:39:26Z

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

Done.

yaooqinn

+1, LGTM. Pending CIs

panbingkun · 2024-06-07T08:44:47Z

+        "url" -> options.getRedactUrl(),
+        "tableName" -> toSQLId(ident)),
+      dialect,
+      description = s"Failed to load table: $ident"


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

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

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

cloud-fan · 2024-06-07T08:49:27Z

+  def checkErrorFailedLoadTable(e: AnalysisException, tbl: String): Unit = {
+    checkError(
+      exception = e,
+      errorClass = "FAILED_JDBC.UNCLASSIFIED",


shouldn't this be LOAD_TABLE?

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))

}

It lost the upstream errorClass in this place

It seems that we should correct it.

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

cloud-fan · 2024-06-07T20:56:15Z

thanks, merging to master!

…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 <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>

[SPARK-46393][SQL][TESTS] Fix UT

9f341af

github-actions Bot added the SQL label Jun 7, 2024

panbingkun mentioned this pull request Jun 7, 2024

[SPARK-46393][SQL][FOLLOWUP] Classify exceptions in JDBCTableCatalog.loadTable #46905

Closed

panbingkun marked this pull request as ready for review June 7, 2024 08:18

update

b8a40c0

panbingkun and others added 2 commits June 7, 2024 16:37

Merge branch 'master' into SPARK-46393_FOLLOWUP

2d136ef

yaooqinn approved these changes Jun 7, 2024

View reviewed changes

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 commented Jun 7, 2024

View reviewed changes

cloud-fan reviewed Jun 7, 2024

View reviewed changes

cloud-fan approved these changes Jun 7, 2024

View reviewed changes

cloud-fan closed this in 8911d59 Jun 7, 2024

panbingkun mentioned this pull request Jun 11, 2024

[SPARK-48585][SQL] Make built-in JdbcDialect's method classifyException throw out the original exception #46937

Closed

	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))
	}

Conversation

panbingkun commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

panbingkun commented Jun 7, 2024

Uh oh!

yaooqinn commented Jun 7, 2024

Uh oh!

panbingkun commented Jun 7, 2024

Uh oh!

panbingkun commented Jun 7, 2024

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

panbingkun commented Jun 7, 2024 •

edited

Loading

panbingkun Jun 7, 2024 •

edited

Loading

panbingkun Jun 7, 2024 •

edited

Loading