cloud-fan commented on code in PR #59297:
URL: https://github.com/apache/spark/pull/59297#discussion_r4226654853
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala:
##########
@@ -72,6 +72,17 @@ object JDBCRDD extends Logging {
try {
getQueryOutputSchema(fullQuery, options, dialect, conn)
} catch {
+ // This must come before the checks below, because some dialects treat
any class-42
+ // SQLSTATE as object-not-found or as a syntax error.
+ case e: SQLException if dialect.isInsufficientPrivilegeException(e) =>
+ throw new SparkException(
+ errorClass = "JDBC_EXTERNAL_ENGINE_INSUFFICIENT_PRIVILEGE",
+ messageParameters = Map(
+ "jdbcQuery" -> fullQuery,
+ "externalEngineError" -> e.getMessage.replaceAll("\\.+$", ""),
Review Comment:
**Non-blocking (P2):** A positive classifier result does not guarantee that
SQLException.getMessage is non-null. PostgreSQL recognizes SQLSTATE 42501
alone, so a reason-less exception reaches this replaceAll call and throws
NullPointerException instead of JDBC_EXTERNAL_ENGINE_INSUFFICIENT_PRIVILEGE,
masking the original diagnostic. The positive fixtures all carry messages, so
this path currently has no regression signal.
**Verification:**
- **Regression:** Exercise a positive SQLSTATE-only privilege exception with
a null reason through schema resolution and assert the dedicated condition plus
preserved diagnostic fields and cause.
- **Behavior:** Confirm existing non-null vendor errors still trim trailing
periods and report the same query and external SQLSTATE.
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -3290,6 +3290,86 @@ class JDBCSuite extends SharedSparkSession {
assert(!dialect.isSyntaxErrorBestEffort(new SQLException("Connection
reset", "08001")))
}
+ test("SPARK-60089: dialects classify remote insufficient-privilege errors") {
+ // (dialect, privilege errors, non-privilege errors)
+ val cases: Seq[(JdbcDialect, Seq[SQLException], Seq[SQLException])] = Seq(
+ (PostgresDialect(),
+ Seq(new SQLException("permission denied for table t", "42501")),
+ Seq(new SQLException("relation does not exist", "42P01"),
+ new SQLException("syntax error", "42601"))),
+ (MySQLDialect(),
+ Seq(1044, 1142, 1143, 1227, 1370).map(code => new
SQLException("denied", "42000", code)),
+ Seq(new SQLException("syntax error", "42000", 1064),
+ new SQLException("unknown table", "42S02", 1146))),
+ (MsSqlServerDialect(),
+ Seq(229, 230, 262, 297, 300).map(code => new SQLException("denied",
"S0001", code)),
+ Seq(new SQLException("invalid object name", "S0002", 208),
+ new SQLException("incorrect syntax", "S0001", 102))),
+ (OracleDialect(),
+ Seq(new SQLException("ORA-01031: insufficient privileges", "42000",
1031)),
+ Seq(new SQLException("ORA-00942: table or view does not exist",
"42000", 942),
+ new SQLException(null: String))),
+ (SnowflakeDialect(),
+ Seq(new SQLException("Insufficient privileges to operate on table",
"42501", 3001)),
+ Seq(new SQLException("does not exist or not authorized", "42S02",
2003))),
+ (TeradataDialect(),
+ Seq(3523, 3524, 5315).map(code => new SQLException("no access",
"42000", code)),
+ Seq(new SQLException("Object does not exist", "42S02", 3807))),
+ (DB2Dialect(),
+ Seq(new SQLException("SQL0551N", "42501", -551),
+ new SQLException("SQL0552N", "42502", -552)),
+ Seq(new SQLException("SQL0204N", "42704", -204))),
+ (DatabricksDialect(),
+ Seq(new SQLException("[INSUFFICIENT_PERMISSIONS] Insufficient
privileges", "42501"),
+ new SQLException("[INSUFFICIENT_PERMISSIONS] Insufficient
privileges", "07000")),
+ Seq(new SQLException("[TABLE_OR_VIEW_NOT_FOUND]", "42P01"),
+ new SQLException(null: String))))
+
+ cases.foreach { case (dialect, privilegeErrors, otherErrors) =>
+ privilegeErrors.foreach(e =>
assert(dialect.isInsufficientPrivilegeException(e), s"$dialect"))
+ (otherErrors :+ new SQLException("connection reset", "08001")).foreach {
e =>
+ assert(!dialect.isInsufficientPrivilegeException(e), s"$dialect")
+ }
+ }
+ assert(!H2Dialect().isInsufficientPrivilegeException(new
SQLException("denied", "42501")))
+ }
+
+ test("SPARK-60089: schema resolution surfaces insufficient-privilege
errors") {
+ // Treat H2 "table not found" as a privilege error. Registering this next
to the built-in H2
+ // dialect also covers AggregatedDialect delegation.
+ val testDialect = new JdbcDialect {
+ override def canHandle(url: String): Boolean = url.startsWith("jdbc:h2")
+ override def isInsufficientPrivilegeException(e: SQLException): Boolean =
+ Set(42102, 42103, 42104).contains(e.getErrorCode)
+ }
+ JdbcDialects.registerDialect(testDialect)
+ try {
+ val e = intercept[SparkException] {
+ spark.read.jdbc(urlWithUserAndPass, "TEST.NO_SUCH_TABLE", new
Properties())
Review Comment:
**Non-blocking (P2):** This V1 read reaches resolveTable with ident=None, so
the object-not-found guard is disabled even though the synthetic H2 exception
matches both classifiers. It therefore does not protect the stated catch
precedence on JDBCTableCatalog.loadTable, where ident is defined; moving
privilege handling below object-not-found would break V2 behavior while this
test remained green.
**Verification:**
- **Regression:** Exercise V2 loadTable with an overlapping
privilege/object-not-found exception and assert the privilege condition,
SQLSTATE, and original cause.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -819,6 +819,13 @@ abstract class JdbcDialect extends Serializable with
Logging {
@Since("4.3.0")
def isNotSelectableObjectException(e: SQLException): Boolean = false
+ /**
+ * Returns true if the given exception indicates that the remote user lacks
a privilege needed to
+ * run the query. Dialects override this to recognize their own error codes;
the default is false.
+ */
+ @Since("4.4.0")
+ def isInsufficientPrivilegeException(e: SQLException): Boolean = false
Review Comment:
**Non-blocking (P2):** JdbcDialect already exposes classifyException for
vendor-specific error mapping, and JdbcUtils wraps schema-loading operations
with it. Adding this second public classifier makes resolveTable consult a
competing path first: for a registered PostgreSQL-compatible dialect that
handles 42501 in classifyException but inherits this new default,
AggregatedDialect can take the built-in positive and throw the new condition
before the custom classifier runs. Please integrate this category with the
established classification contract so custom-first behavior and built-in
fallback have one precedence model.
**Recommended change:** Implement insufficient-privilege mapping through the
existing JdbcDialect.classifyException API, refactor its aggregation to
distinguish unhandled results from definitive custom classifications, and
remove the parallel public Boolean hook. Preserve
privilege-before-object-not-found/syntax behavior at schema resolution while
routing the final mapping through one classification contract.
**Why this works:** Give the established classification path an explicit
internal notion of unhandled versus classified, compose matching dialects in
documented order, and have JDBCRDD/JdbcUtils consume the resulting
SparkThrowable. Built-in privilege mappings become classifyException cases
rather than implementations of a separate predicate.
**Scope:** Add the new category to Spark's established dialect
classification path instead of creating a competing public API.
**Compatibility:** Vendor code coverage, structured error parameters, and
custom-without-opinion built-in fallback remain unchanged.
**Risks:** Refactoring the established DeveloperApi classification path must
preserve source and binary compatibility where required. Incorrect unhandled
detection could mask built-in mappings or override custom classifications.
**Constraints:** An existing custom classifyException result must not be
bypassed by a later built-in dialect. A custom dialect with no relevant
classification must still fall through to the built-in mapping. Privilege
classification must retain precedence over overlapping object-not-found and
syntax classifications during schema resolution.
**Success:** JdbcDialect exposes one authoritative exception-classification
contract to external implementors. A registered custom classifyException result
wins over later built-ins, while an unhandled result falls through. Built-in
insufficient-privilege errors still produce
JDBC_EXTERNAL_ENGINE_INSUFFICIENT_PRIVILEGE during schema resolution.
**Verification:**
- **Compatibility:** Register custom dialects with a definitive
classifyException result and with no relevant result ahead of a matching
built-in, then verify custom-first behavior and built-in fallback.
- **Behavior:** Verify every built-in vendor privilege vector still reaches
the dedicated condition through the unified classification path.
- **Regression:** Verify overlapping privilege, object-not-found, and syntax
signals retain the intended schema-resolution precedence.
--
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]
