urosstan-db commented on code in PR #58862:
URL: https://github.com/apache/spark/pull/58862#discussion_r4039031426
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/SharedJDBCIntegrationSuite.scala:
##########
@@ -63,14 +63,25 @@ abstract class SharedJDBCIntegrationSuite extends
DockerJDBCIntegrationSuite {
// Exception should be detected in analysis phase first when we resolve a
schema from
// through JDBC by sending SELECT * FROM (<subquery>) [LIMIT 1][WHERE 1=0]
query.
- checkErrorMatchPVals(
- ex,
+ checkError(
+ exception = ex,
condition =
"JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR.DURING_OUTPUT_SCHEMA_RESOLUTION",
+ sqlState = Some("42000"),
parameters = Map(
"jdbcQuery" -> "SELECT \\* FROM \\(.*",
- "externalEngineError" -> "[\\s\\S]*"
- )
+ "externalEngineError" -> "[\\s\\S]+",
+ "externalEngineSqlState" -> ".+"
+ ),
+ matchPVals = true
)
+ ex.getCause match {
+ case cause: SQLException =>
+ val expectedSqlState =
+ Option(cause.getSQLState).filter(_.nonEmpty).getOrElse("unknown")
+ assert(ex.getMessageParameters.get("externalEngineSqlState") ===
expectedSqlState)
Review Comment:
nit: we may use assertResult for better error handling
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala:
##########
@@ -119,6 +120,11 @@ object JDBCRDD extends Logging {
}
}
+ // Treat null or blank driver SQLSTATE as missing.
+ private def jdbcExternalEngineSqlState(e: SQLException): String = {
Review Comment:
nit: We should name the method per its functionality, not by external use
cases (external engine is not relevant keyword here)
```suggestion
/**
* Gets SQL state of provided sql exception or "unknown" if sql state is
not defined.
*/
private def getSQLState(e: SQLException): String = {
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala:
##########
@@ -119,6 +120,11 @@ object JDBCRDD extends Logging {
}
}
+ // Treat null or blank driver SQLSTATE as missing.
+ private def jdbcExternalEngineSqlState(e: SQLException): String = {
Review Comment:
Consider moving it to JDBCUtils, I am fine with current place as well
##########
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. The error was caused by the query
<jdbcQuery>. <externalEngineError>. External engine SQLSTATE:
<externalEngineSqlState>."
Review Comment:
I would put external engine SQL state before external engine error, since
external engine error can be long.
--
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]
