Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,35 @@ private case class MySQLDialect() extends JdbcDialect with SQLConfHelper with No

// See https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html
override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = {
"42000".equals(exception.getSQLState)
"42000".equals(exception.getSQLState) &&
!isNonSyntaxErrorBestEffort(exception)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// See https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html
// MySQL uses SQLSTATE 42000 for both syntax errors and access/limit failures.
private def isNonSyntaxErrorBestEffort(exception: SQLException): Boolean = {
val nonSyntaxErrorCodes = Set(
1044, // ER_DBACCESS_DENIED_ERROR
1142, // ER_TABLEACCESS_DENIED_ERROR
1143, // ER_COLUMNACCESS_DENIED_ERROR
1148, // ER_NOT_ALLOWED_COMMAND
1203, // ER_TOO_MANY_USER_CONNECTIONS
1226, // ER_USER_LIMIT_REACHED
1227, // ER_SPECIFIC_ACCESS_DENIED_ERROR
1370 // ER_PROCACCESS_DENIED_ERROR
)
// Message matching is a best-effort fallback for drivers that omit error codes.
// Prefer error codes above; MySQL messages may be localized.
val nonSyntaxSubstrings = Set(
"command denied to user",
"access denied",
"must have privileges"
)
val message = Option(exception.getMessage)
.map(_.toLowerCase(Locale.ROOT))
.getOrElse("")
nonSyntaxErrorCodes.contains(exception.getErrorCode) ||
nonSyntaxSubstrings.exists(message.contains)
}

// See https://dev.mysql.com/doc/refman/8.0/en/alter-table.html
Expand Down
68 changes: 68 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2746,6 +2746,74 @@ class JDBCSuite extends SharedSparkSession {
assert(!dialect.isSyntaxErrorBestEffort(new SQLException("Connection reset", "08001")))
}

test("MySQLDialect syntax error detection") {
val dialect = MySQLDialect()
assert(dialect.isSyntaxErrorBestEffort(
new SQLException(
"You have an error in your SQL syntax; check the manual that corresponds to your " +
"MySQL server version for the right syntax to use near 'SELCT' at line 1",
"42000",
1064)))
// Permission / limit failures share SQLSTATE 42000; detect by error code.
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"Access denied for user 'testuser'@'%' to database 'testdb'",
"42000",
1044)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"SELECT command denied to user 'testuser'@'%' for table 't1'",
"42000",
1142)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"SELECT command denied to user 'testuser'@'%' for column 'c1' in table 't1'",
"42000",
1143)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"The used command is not allowed with this MySQL version",
"42000",
1148)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"User testuser already has more than 'max_user_connections' active connections",
"42000",
1203)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"User 'testuser' has exceeded the 'max_queries_per_hour' resource (current value: 1000)",
"42000",
1226)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"Access denied; you need (at least one of) the SUPER privilege(s) for this operation",
"42000",
1227)))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"execute command denied to user 'testuser'@'%' for routine 'p1'",
"42000",
1370)))
// Best-effort message fallback when the vendor error code is absent.
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"execute command denied to user 'testuser'@'%' for table 't1'",
"42000")))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"You must have privileges to update tables in the mysql database to be able to " +
"change passwords for others",
"42000")))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(
"Access denied for user 'testuser'@'%' to database 'testdb'",
"42000")))
assert(!dialect.isSyntaxErrorBestEffort(
new SQLException(null, "42000", 1142)))
assert(!dialect.isSyntaxErrorBestEffort(new SQLException("Connection reset", "08001")))
}

test("SPARK-45425: Mapped TINYINT to ShortType for MsSqlServerDialect") {
val msSqlServerDialect = JdbcDialects.get("jdbc:sqlserver")
val metadata = new MetadataBuilder().putLong("scale", 1)
Expand Down