uros-b commented on code in PR #58862:
URL: https://github.com/apache/spark/pull/58862#discussion_r4035157496
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -5741,7 +5741,7 @@
},
"JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR" : {
"message" : [
- "JDBC external engine syntax error. The error was caused by the query
<jdbcQuery>. <externalEngineError>."
+ "JDBC external engine syntax error with SQLSTATE
<externalEngineSqlState>. The error was caused by the query <jdbcQuery>.
<externalEngineError>."
Review Comment:
The change adds externalEngineSqlState to both
JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR throw sites in JDBCRDD and templates it as:
```
JDBC external engine syntax error with SQLSTATE <externalEngineSqlState>.
The error was caused by the query <jdbcQuery>. <externalEngineError>.
```
Keeping Spark’s own SQLSTATE at 42000 and putting the remote value in
messageParameters is the correct design. Putting that value in the message as
bare “SQLSTATE” fights Spark’s existing pretty-printer, which already appends
Spark’s SQLSTATE:
```
SparkThrowableHelper.scala
Ln 78–86
def formatErrorMessage(
errorClass: String,
displayMessage: String,
sqlState: String,
context: String): String = {
val displaySqlState = if (sqlState == null) "" else s" SQLSTATE: $sqlState"
val displayQueryContext = (if (context.isEmpty) "" else "\n") + context
val prefix = if (errorClass.startsWith("_LEGACY_ERROR_")) "" else
s"[$errorClass] "
s"$prefix$displayMessage$displaySqlState$displayQueryContext"
}
```
On Postgres (42601) the user-visible line becomes two different SQLSTATEs:
```
[JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.DURING_OUTPUT_SCHEMA_RESOLUTION] JDBC
external engine
syntax error with SQLSTATE 42601. The error was caused by the query ....
<engine message>. The error occurred during output schema resolution. SQLSTATE:
42000
```
That undercuts the grouping/observability goal: log parsers and humans
cannot tell which SQLSTATE is Spark’s and which is the engine’s.
Suggested template:
```
JDBC external engine syntax error. The error was caused by the query
<jdbcQuery>. <externalEngineError>. External engine SQLSTATE:
<externalEngineSqlState>.
```
The placeholder has to stay in the template (tests reject unused parameters
unless the class is allowlisted). Only the label needs to change.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala:
##########
@@ -87,7 +87,8 @@ object JDBCRDD extends Logging {
errorClass =
"JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.DURING_OUTPUT_SCHEMA_RESOLUTION",
messageParameters = Map(
"jdbcQuery" -> fullQuery,
- "externalEngineError" -> e.getMessage.replaceAll("\\.+$", "")
+ "externalEngineError" -> e.getMessage.replaceAll("\\.+$", ""),
+ "externalEngineSqlState" ->
Option(e.getSQLState).getOrElse("unknown")
Review Comment:
Treat blank SQLSTATE as missing. Option(e.getSQLState).getOrElse("unknown")
leaves "" in the message (with SQLSTATE . ...). Prefer
Option(e.getSQLState).filter(_.nonEmpty).getOrElse("unknown"). Most dialects
that wrap this error already have a class-42 SQLSTATE; SQL Server is the
outlier (message-based detection), so this fallback actually matters.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/SharedJDBCIntegrationSuite.scala:
##########
@@ -68,9 +68,12 @@ abstract class SharedJDBCIntegrationSuite extends
DockerJDBCIntegrationSuite {
condition =
"JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.DURING_OUTPUT_SCHEMA_RESOLUTION",
parameters = Map(
"jdbcQuery" -> "SELECT \\* FROM \\(.*",
- "externalEngineError" -> "[\\s\\S]*"
+ "externalEngineError" -> "[\\s\\S]*",
+ "externalEngineSqlState" -> "[\\s\\S]*"
)
)
+ assert(ex.getMessageParameters.get("externalEngineSqlState") ===
+
Option(ex.getCause.asInstanceOf[SQLException].getSQLState).getOrElse("unknown"))
}
Review Comment:
Test is in the right suite, but weak. SharedJDBCIntegrationSuite is the
existing SPARK-52184 coverage and will run in CI because this PR touches
docker-integration-tests. Please tighten it:
- assert Spark SQLSTATE is still 42000 (sqlState = Some("42000"))
- avoid asInstanceOf[SQLException] on getCause
- [\\s\\S]* accepts an empty value; the extra assert is what actually checks
the parameter
An H2 test in JDBCSuite would cover this without Docker; not required if the
Docker job is green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
