[SPARK-58515][SQL] Exclude MySQL permission and limit errors from syntax error classification - #57720
Conversation
…ssification MySQL reports permission-denied and resource-limit failures with SQLState 42000, the same state used for syntax errors. Narrow isSyntaxErrorBestEffort so those cases are not treated as syntax errors.
alekjarmov
changed the title
Prefer vendor error codes for access and limit failures, keep English message matching as a fallback, and null-safe the message check.
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
1 blocking, 0 non-blocking, 0 nits.
The targeted exclusions help, but the denylist approach still violates the syntax-classification contract for documented MySQL errors.
Correctness (1)
- sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:225: The non-syntax denylist remains incomplete: documented MySQL error 1049 (
Unknown database) also uses SQLSTATE 42000, so the method still returns true for a non-syntax error. -- see inline
Verification
I traced the override against JdbcDialect's contract that a true result is guaranteed to be a syntax error. MySQL's official error reference documents ER_BAD_DB_ERROR (1049) as SQLSTATE 42000 with message Unknown database; because 1049 is not excluded, the changed implementation still returns true for that non-syntax error. Tests were not run as part of this review.
PR description suggestions
- Document: why the chosen classification strategy is complete enough to satisfy JdbcDialect's true-means-syntax guarantee, including how other non-syntax 42000 vendor codes are handled.
| override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = { | ||
| "42000".equals(exception.getSQLState) | ||
| "42000".equals(exception.getSQLState) && | ||
| !isNonSyntaxErrorBestEffort(exception) |
There was a problem hiding this comment.
true must mean this is confidently a syntax error, but the denylist still lets documented non-syntax 42000 errors through. For example, ER_BAD_DB_ERROR (1049) is 42000 with message Unknown database, so this path returns true. Please use an allowlist of vendor codes known to represent syntax errors (including 1064), and add 1049 as a regression case.
There was a problem hiding this comment.
The problem with this is that there are 144 errors with "42000" errors and the majority of them are syntax errors , so if we go with the allowlist approach we either have some allowlist of 80 entires which seems like too much to maintain or miss a lot of them. I think this would be more accurate in general but I can swap to allowlist if you still think it's better.
There was a problem hiding this comment.
That is a fair concern; maintaining an exhaustive allowlist of roughly 80 codes would not be attractive. I do not think it needs to be exhaustive, though. This API deliberately permits false negatives (it may fail to detect some syntax errors), while its contract forbids false positives (true must guarantee a syntax error). So I would start with a small allowlist of codes we are confident about, such as 1064, and add others only when needed with focused tests. Unknown 42000 codes would return false. That preserves the contract without requiring us to classify every MySQL error up front.
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
+1 with @cloud-fan's denylist concern in MySQLDialect, otherwise looks good - thank you @alekjarmov!
3 participants
MySQL reports permission-denied and resource-limit failures with SQLState 42000, the same state used for syntax errors. Narrow isSyntaxErrorBestEffort so those cases are not treated as syntax errors.
What changes were proposed in this pull request?
Improves the syntax error classification to not include some of the errors with sqlState "42000" but which are not syntax errors, rather permission.
Why are the changes needed?
Fix wrongly classified errors.
Does this PR introduce any user-facing change?
How was this patch tested?
Added a unit test.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Opus 5