[SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException #44358

MaxGekk · 2023-12-14T15:18:05Z

What changes were proposed in this pull request?

In the PR, I propose to raise exceptions with only error classes from JdbcUtils.classifyException, and introduce new error class FAILED_JDBC with sub-classes linked to a particular JDBC operation.

Why are the changes needed?

To improve user experience with Spark SQL by migrating on new error framework when all Spark exceptions from the JDBC datasource have an error class.

Does this PR introduce any user-facing change?

Yes, if user's code depends on exceptions from the JDBC datasource.

How was this patch tested?

By running the affected test suites:

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

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

No.

beliefer · 2023-12-15T10:31:44Z

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

-            val tableName = regex.findFirstMatchIn(message).get.group(2)
+          case 42111 if errorClass == "FAILED_JDBC.CREATE_INDEX" =>
+            val indexName = messageParameters("indexName")
+            val tableName = messageParameters("tableName")


Good improvement!

beliefer · 2023-12-15T10:33:49Z

common/utils/src/main/resources/error/error-classes.json

@@ -1096,6 +1096,79 @@
    ],
    "sqlState" : "38000"
  },
+  "FAILED_JDBC" : {


FAILED_JDBC -> FAILED_JDBC_OPERATION

Too long, does _OPERATION bring any benefits?

Just because FAILED_JDBC looks confused.

JDBC means Java Database Connectivity, the connectivity might fails at any time, ops or while establishing connections or while keeping it. For instance, this is real example:

org.postgresql.util.PSQLException: The connection attempt failed. at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:331) at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49) at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:223)

I guess the connectivity is not changed if create index failed

beliefer

LGTM.

beliefer · 2023-12-15T11:18:33Z

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

+            val newTable = messageParameters("newName")
+            throw QueryCompilationErrors.tableAlreadyExistsError(newTable)
+          case "42P07" if pgAlreadyExistsRegex.findFirstMatchIn(sqlException.getMessage).nonEmpty =>
+            val tableName = pgAlreadyExistsRegex.findFirstMatchIn(sqlException.getMessage)


Please make pgAlreadyExistsRegex.findFirstMatchIn(sqlException.getMessage) execute only once

srielau · 2023-12-15T18:21:50Z

common/utils/src/main/resources/error/error-classes.json

+        ]
+      }
+    },
+    "sqlState" : "40000"


That would be "transaction rollback"
This is all about reaching out to foreign data right?
HV000 FDW-specific condition?

Is it obvious from the context what we are connecting to? Should we add some JDBC level context (URL?)

This is all about reaching out to foreign data right?

Right.

HV000 FDW-specific condition?

Seems like, but what do you propose? to get sqlState from SQLExpression?

Is it obvious from the context what we are connecting to?

Should be if an user reads data from one JDBC datasource.

Should we add some JDBC level context (URL?)

I can extract and pass option.url to AnalysisException with new error class FAILED_JDBC but how about other (classified exceptions) like:

case "42P07" => if (errorClass == "FAILED_JDBC.CREATE_INDEX") { throw new IndexAlreadyExistsException( indexName = messageParameters("indexName"), tableName = messageParameters("tableName"), cause = Some(e))

The IndexAlreadyExistsException has its own error class INDEX_ALREADY_EXISTS without any parameters related to JDBC url.

I was proposing to replace 40000 with HV004.

This example is interesting. How do we get a 42P07 with FAILED_JDBC errorclass?

I was proposing to replace 40000 with HV004.

@srielau I didn't get the logic why HV004 but not HV000? In postgresql for instance, it is related to fdw_invalid_data_type

How do we get a 42P07 with FAILED_JDBC errorclass?

From the doc https://www.postgresql.org/docs/14/errcodes-appendix.html and experiments.

Sorry, I fat-fingered. Yes HV000

…yException

MaxGekk · 2023-12-16T17:32:13Z

@srielau @cloud-fan I am going to merge this PR soon if you don't mind.

MaxGekk · 2023-12-17T07:40:27Z

Merging to master. Thank you, @beliefer @srielau for review.

cloud-fan · 2023-12-18T09:10:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

@@ -1180,12 +1180,15 @@ object JdbcUtils extends Logging with SQLConfHelper {
    }
  }

-  def classifyException[T](message: String, dialect: JdbcDialect)(f: => T): T = {
+  def classifyException[T](


this is a breaking change, isn't it? JDBCDialect is a public developer API.

when was this API added? @beliefer do you know?

Perhaps we should keep two versions.

And how will you combine exceptions from the both versions?

there are no nested exception here. We pass a default error class to classifyException and if a dialect cannot classify an exception from JDBC driver, it throws the default one otherwise it forms and throws another one. The exception from JDBC driver is added as cause.

So if a dialect can classify an exception, we will lose the error class which is actually worse?

Can we also enable MiMa for this? I believe this is being skipped because it's under execution package.

So if a dialect can classify an exception, we will lose the error class which is actually worse?

@cloud-fan A dialect can override proposed error class and make our default error class more precise using the driver specific info. And yes, the original error class will be lost.

Can we also enable MiMa for this? I believe this is being skipped because it's under execution package.

@HyukjinKwon I made the modification because MiMa complained. Did you change MiMa to expect different behaviour?

I think losing the original error class is bad. We added classifyException for getting more precise java exception type, but I don't think this is useful anymore as we have error classes. My suggestion is to deprecate classifyException, saying that Spark never calls it anymore.

…ssifyException` ### What changes were proposed in this pull request? In the PR, I propose to restore `classifyException()` of `JdbcDialect` before the commit 14a933b, and extends `classifyException()` with the error class parameter by `description`: ```scala def classifyException( e: Throwable, errorClass: String, messageParameters: Map[String, String], description: String): AnalysisException ``` The `description` parameter has the same meaning as `message` in the old version of `classifyException()` which is deprecated. Also old implementation of `classifyException()` has been restored in JDBC dialects: MySQL, PostgreSQL and so on. ### Why are the changes needed? To restore compatibility with existing JDBC dialects. ### Does this PR introduce _any_ user-facing change? No, this PR restores the behaviour prior #44358. ### How was this patch tested? By running the affected test suite: ``` $ build/sbt "core/testOnly *SparkThrowableSuite" ``` and modified test suite: ``` $ build/sbt "test:testOnly *JDBCV2Suite" $ build/sbt "test:testOnly *JDBCTableCatalogSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #44449 from MaxGekk/restore-jdbc-classifyException. Authored-by: Max Gekk <[email protected]> Signed-off-by: Max Gekk <[email protected]>

…or classes ### What changes were proposed in this pull request? In the PR, I propose to port the existing `classifyException()` method which accepts a description to new one w/ an error class added by #44358. The modified JDBC dialects are: DB2, H2, Oracle, MS SQL Server, MySQL and PostgreSQL. ### Why are the changes needed? The old method `classifyException()` which accepts a `description` only has been deprecated already by ... ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By existing integration tests, and the modified test suite: ``` $ build/sbt "test:testOnly *JDBCV2Suite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #44739 from MaxGekk/port-jdbc-classifyException. Authored-by: Max Gekk <[email protected]> Signed-off-by: Max Gekk <[email protected]>

Assign error classes/subclasses to JdbcUtils.classifyException

50a7936

github-actions bot added SQL DOCS labels Dec 14, 2023

MaxGekk added 5 commits December 14, 2023 19:02

Re-gen sql-error-conditions-failed-jdbc-error-class.md

8c6158e

Fix PostgresDialect

d7302f2

Exclude classifyException from Mima

d3204c7

Port some tests to checkError

f9072ae

Fix intergration tests

901c511

github-actions bot added the BUILD label Dec 14, 2023

MaxGekk added 3 commits December 14, 2023 23:29

Use checkError

9b0090f

Quoting

be74430

Fix integration tests

c9a23f5

MaxGekk changed the title ~~[WIP][SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException~~ [SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException Dec 15, 2023

MaxGekk marked this pull request as ready for review December 15, 2023 09:48

MaxGekk mentioned this pull request Dec 15, 2023

[SPARK-46393][SQL] Classify exceptions in the JDBC table catalog #44335

Closed

MaxGekk requested review from beliefer, cloud-fan and huaxingao December 15, 2023 09:50

beliefer reviewed Dec 15, 2023

View reviewed changes

beliefer approved these changes Dec 15, 2023

View reviewed changes

MaxGekk added 3 commits December 15, 2023 15:47

Fix test

707c212

Address a comment

d21fb1e

Fix integration tests 2

2e8eeb0

srielau reviewed Dec 15, 2023

View reviewed changes

MaxGekk added 3 commits December 16, 2023 09:38

Add JDBC url

01a1a9c

Re-gen *.md

0338fbc

Merge remote-tracking branch 'origin/master' into error-class-classif…

06fbd90

…yException

MaxGekk closed this in 14a933b Dec 17, 2023

cloud-fan reviewed Dec 18, 2023

View reviewed changes

MaxGekk mentioned this pull request Dec 21, 2023

[SPARK-46608][SQL] Restore backward compatibility of JdbcDialect.classifyException #44449

Closed

MaxGekk mentioned this pull request Jan 15, 2024

[SPARK-46727][SQL] Port classifyException() in JDBC dialects on error classes #44739

Closed

panbingkun mentioned this pull request Jun 12, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException #44358

[SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException #44358

MaxGekk commented Dec 14, 2023 •

edited

Loading

beliefer Dec 15, 2023

beliefer Dec 15, 2023

MaxGekk Dec 15, 2023

beliefer Dec 15, 2023

MaxGekk Dec 15, 2023

beliefer Dec 15, 2023

beliefer left a comment

beliefer Dec 15, 2023

srielau Dec 15, 2023 •

edited

Loading

MaxGekk Dec 15, 2023

srielau Dec 15, 2023

MaxGekk Dec 16, 2023 •

edited

Loading

srielau Dec 16, 2023

MaxGekk commented Dec 16, 2023

MaxGekk commented Dec 17, 2023

cloud-fan Dec 18, 2023

cloud-fan Dec 18, 2023

MaxGekk Dec 18, 2023

beliefer Dec 18, 2023

MaxGekk Dec 18, 2023

MaxGekk Dec 18, 2023

cloud-fan Dec 19, 2023

HyukjinKwon Dec 19, 2023

MaxGekk Dec 19, 2023

cloud-fan Dec 19, 2023

[SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException #44358

[SPARK-46410][SQL] Assign error classes/subclasses to JdbcUtils.classifyException #44358

Conversation

MaxGekk commented Dec 14, 2023 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srielau Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Dec 16, 2023

MaxGekk commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Dec 14, 2023 •

edited

Loading

srielau Dec 15, 2023 •

edited

Loading

MaxGekk Dec 16, 2023 •

edited

Loading