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

[panbingkun]

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:

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

Lines 746 to 751 in df4156a

	def classifyException(
	e: Throwable,
	errorClass: String,
	messageParameters: Map[String, String],
	description: String): AnalysisException = {
	classifyException(description, e)

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.

[panbingkun]

From the code logic, it seems that there is no need for the field description to exist. Let's remove it.
Although JdbcDialect is marked as DeveloperApi, this method has been added from Spark version 4.0 and the version 4.0 has not been released yet.
Can we directly remove it?

[cloud-fan]

I think there was a reason to call the legacy classifyException method here. Can we dig into it?

[panbingkun]

I am investigating history.

[yaooqinn]

This guarantees pre-implemented legacy classifyExceptions from third-party to be correctly called

[panbingkun]

Yeah, that's right, see PR below:
1.#44358, the modification of this PR resulted in break changes, and with the suggestion of cloud-fan:

the following PR has been created.

2.#44449, this PR has restored compatibility behavior

[panbingkun]

To reduce confusion among reviewers, this PR does not include the function of adding the prefix catalog info to ident discussed by https://github.com/apache/spark/pull/46912/files#r1630859747
It will be completed as a separate PR.

[panbingkun]

cc @cloud-fan @yaooqinn

[beliefer]

AnalysisException seems unreasonable here.

[panbingkun]

Yeah, this should already be in the execution phase, so it's not reasonable.
Let me go through the history first.

[panbingkun]

• The idea of this PR is to make various built-in JDBC dialects overload the method def classifyException(e: Throwable, errorClass: String, messageParameters: Map[String, String], description: String): AnalysisException and make them call final def classifyException(e: Throwable, errorClass: String, messageParameters: Map[String, String]): AnalysisException, so that the original exception can be thrown.

• Another approach is to define a trait as follows:

trait JdbcDialectHelper extends JdbcDialect {
  
  override def classifyException(
      e: Throwable,
      errorClass: String,
      messageParameters: Map[String, String],
      description: String): AnalysisException = {
    new AnalysisException(
      errorClass = errorClass,
      messageParameters = messageParameters,
      cause = Some(e))
  }
}

eg:

private[sql] case class H2Dialect() extends JdbcDialect with `JdbcDialectHelper` {

}

and then have various built-in jdbc dialects with it, so that there is no need to make changes to each built-in jdbc dialects dialect, but this will add an extra trait.

WDYT @cloud-fan @yaooqinn @beliefer ?

[panbingkun]

• Another approach is to define a trait as follows:

trait JdbcDialectHelper extends JdbcDialect {
  
  override def classifyException(
      e: Throwable,
      errorClass: String,
      messageParameters: Map[String, String],
      description: String): AnalysisException = {
    new AnalysisException(
      errorClass = errorClass,
      messageParameters = messageParameters,
      cause = Some(e))
  }
}

eg:

private[sql] case class H2Dialect() extends JdbcDialect with `JdbcDialectHelper` {

}

and then have various built-in jdbc dialects with it, so that there is no need to make changes to each built-in jdbc dialects dialect, but this will add an extra trait.

I have already used this

[cloud-fan]

can we give it a clearer name? like NoLegacyJDBCError.

[panbingkun]

Okay, let me update it.
Done.

[LuciferYang]

Merged into master for Spark 4.0. Thanks @panbingkun @cloud-fan @yaooqinn @beliefer

[panbingkun]

Merged into master for Spark 4.0. Thanks @panbingkun @cloud-fan @yaooqinn @beliefer

Thanks all.