Conversation
…sification ### What changes were proposed in this pull request? Adds integration coverage for the JDBC external-engine syntax error classification introduced by SPARK-52184 and tightened for PostgreSQL by SPARK-59336, and fixes the dialects the new checks expose. - `SharedJDBCIntegrationSuite` gets two shared checks: reading a non-existent table and reading a table as a user without the read privilege. Both require the driver's own `SQLException` to surface instead of `JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR`. Dialect suites plug in the driver's SQLSTATE and, for the privilege check, a restricted user. - The two SPARK-59336 tests in `PostgresIntegrationSuite` are folded into the shared framework. PostgreSQL, SQL Server and Oracle set up a restricted user. MySQL is left to SPARK-58515 and DB2 cannot create a restricted user over JDBC (its users are OS users); both are documented in place. The StarRocks check is disabled because StarRocks reports a missing table as error 1064 with SQLSTATE 42000, the SQLSTATE it also uses for syntax errors, and it shares `MySQLDialect`. - `DB2Dialect` only classifies SQLSTATE 42601 as a syntax error. Class 42 also covers 42704 (undefined object) and 42501 (insufficient privilege), which were misclassified. - `OracleDialect` keeps the ORA errors that share SQLSTATE 42000 but are not syntax errors (missing object, non-selectable object, missing privilege) out of the classification. - Unit tests for the two dialect classifiers in `JDBCSuite`. ### Why are the changes needed? `JdbcDialect.isSyntaxErrorBestEffort` promises that `true` means a syntax error. Class 42 also covers missing tables, missing privileges and missing columns, so those failures were reported as `JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR`, hiding what actually went wrong. SPARK-59336 did this for PostgreSQL; this PR covers the remaining dialects and moves the checks into the shared integration suite so every dialect is covered and regressions are caught. ### Does this PR introduce _any_ user-facing change? Yes. With DB2 and Oracle, errors that are not syntax errors, such as a missing table or a missing privilege, are no longer reported as `JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR`; the original driver exception is thrown instead. Syntax errors are reported as before. ### How was this patch tested? - `build/sbt -Pdocker-integration-tests docker-integration-tests/Test/compile` - `build/sbt "sql/testOnly org.apache.spark.sql.jdbc.JDBCSuite"` (157 tests) - `build/sbt "sql/testOnly org.apache.spark.sql.jdbc.PostgresDialectSuite"` - `sql/scalastyle`, `sql/Test/scalastyle`, `docker-integration-tests/Test/scalastyle` - The new docker integration checks need `ENABLE_DOCKER_INTEGRATION_TESTS=1` and the database images, so they were not executed locally. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: DeepSeek Harness (deepseek-v4.1-flash)
uros-b
reviewed
Sep 30, 2026
uros-b
left a comment
uros-b
left a comment
Member
There was a problem hiding this comment.
Thank you @Admaing for working on this, upon first look this looks like a well scoped extension of #58621.
@urosstan-db could you please help review these changes?
urosstan-db
reviewed
Sep 30, 2026
| override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = { | ||
| "42000".equals(exception.getSQLState) | ||
| "42000".equals(exception.getSQLState) && | ||
| !nonSyntaxErrorCodes.contains(exception.getErrorCode) |
Contributor
There was a problem hiding this comment.
We miss some things such as:
ORA-00980 (invalid synonym target) and ORA-01775 (synonym loop).
Btw, is it possible to use allowlist for classification instead of disallow list?
Author
There was a problem hiding this comment.
Done in c72ae07 — switched to an allowlist of the documented SQL parsing codes; JDBCSuite now asserts ORA-00980 and ORA-01775 are not classified.
urosstan-db
reviewed
Sep 30, 2026
| // (undefined object) and 42501 (insufficient privilege). | ||
| override def isSyntaxErrorBestEffort(exception: SQLException): Boolean = { | ||
| Option(exception.getSQLState).exists(_.startsWith("42")) | ||
| "42601".equals(exception.getSQLState) |
Contributor
There was a problem hiding this comment.
Are you sure we listed all things:
42602, 42603 and 42604 seems like strong candidates as well, check this list:
https://www.ibm.com/docs/en/db2-for-zos/12.0.0?topic=codes-sqlstate-values-common-error#db2z_sqlstatevalues__classcode42
Author
There was a problem hiding this comment.
Done — now accepts the whole 426xx subclass (42601–42604, ...), with 425xx/427xx excluded; unit tests updated.
urosstan-db
reviewed
Sep 30, 2026
| * SQLSTATE it also uses for syntax errors (e.g. StarRocks reports error 1064/SQLSTATE 42000 for | ||
| * both) cannot pass this check until its classifier is tightened. | ||
| */ | ||
| protected def nonExistentTableIsNotSyntaxError: Boolean = true |
Contributor
There was a problem hiding this comment.
Since we use this concept only in StarRocks suite currently, it is better to add test to excluded list (override excluded method), and specify reason you already did.
Having abstract method for single exclusion polutes base suite interface, if we have some other DB in the future like StarRocks, we may consider refactoring
Author
There was a problem hiding this comment.
Done — the base-suite flag is gone; StarRocks excludes the two checks throughexcludedand keeps the reason there.
- Oracle: classify only the ORA codes documented as SQL parsing errors instead of excluding known non-syntax codes, so the classifier cannot accept a non-syntax error (e.g. ORA-00980 invalid synonym target, ORA-01775 synonym loop). - DB2: the 426xx SQLSTATE subclass is DB2's syntax error family (42601, 42602, 42603, 42604, ...), not only 42601. - SharedJDBCIntegrationSuite: drop the single-use nonExistentTableIsNotSyntaxError flag from the base suite; StarRocks excludes the shared checks through the existing `excluded` mechanism with the reason documented in place. - MySQL and DB2 use `excluded` for the privilege check as well, instead of empty createRestrictedUser overrides. - Unit tests cover DB2 426xx and the ORA codes named in review. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: DeepSeek Harness (deepseek-v4.1-flash)
Member
|
cc @srielau FYI |
- Create the restricted user idempotently in the PostgreSQL, Oracle and SQL Server suites (drop first; an existence check where the driver has no DROP ... IF EXISTS), with the same wording for the reason. - Name the SQL Server database explicitly (databaseName=master) instead of relying on the login's default database. - Restore the PostgreSQL "permission denied" message assertion through an optional expectedMessage field on RestrictedUser. - Point the Oracle classifier comment at the current Database Error Messages manual and mention the ORA-017xx range as well. - Note that the `assume` in the privilege test is a guard for a suite that neither creates a user nor excludes the test. Verified against real containers (ENABLE_DOCKER_INTEGRATION_TESTS=1, DOCKER_IP=127.0.0.1): - PostgresIntegrationSuite -z SPARK-59369: 2/2, missing table 42P01 and privilege 42501 with "permission denied". - PostgresIntegrationSuite -z SPARK-52184: 1/1. - MySQLIntegrationSuite -z SPARK-59369: missing table 42S02 passes and the privilege check is reported as excluded/ignored. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: DeepSeek Harness (deepseek-v4.1-flash)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Adds integration coverage for the JDBC external-engine syntax error classification
introduced by SPARK-52184 and tightened for PostgreSQL by SPARK-59336, and fixes the
dialects the new checks expose.
SharedJDBCIntegrationSuitegets two shared checks: reading a non-existent table andreading a table as a user without the read privilege. Both require the driver's own
SQLExceptionto surface instead ofJDBC_EXTERNAL_ENGINE_SYNTAX_ERROR. Dialect suitesplug in the driver's SQLSTATE and, for the privilege check, a restricted user.
PostgresIntegrationSuiteare folded into the sharedframework. PostgreSQL, SQL Server and Oracle set up a restricted user. MySQL is left to
SPARK-58515 and DB2 cannot create a restricted user over JDBC (its users are OS users);
both are documented in place. The StarRocks check is disabled because StarRocks reports a
missing table as error 1064 with SQLSTATE 42000, the SQLSTATE it also uses for syntax
errors, and it shares
MySQLDialect.DB2Dialectonly classifies SQLSTATE 42601 as a syntax error. Class 42 also covers 42704(undefined object) and 42501 (insufficient privilege), which were misclassified.
OracleDialectkeeps the ORA errors that share SQLSTATE 42000 but are not syntax errors(missing object, non-selectable object, missing privilege) out of the classification.
JDBCSuite.Why are the changes needed?
JdbcDialect.isSyntaxErrorBestEffortpromises thattruemeans a syntax error. Class 42also covers missing tables, missing privileges and missing columns, so those failures were
reported as
JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR, hiding what actually went wrong. SPARK-59336did this for PostgreSQL; this PR covers the remaining dialects and moves the checks into the
shared integration suite so every dialect is covered and regressions are caught.
Does this PR introduce any user-facing change?
Yes. With DB2 and Oracle, errors that are not syntax errors, such as a missing table or a
missing privilege, are no longer reported as
JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR; theoriginal driver exception is thrown instead. Syntax errors are reported as before.
How was this patch tested?
build/sbt -Pdocker-integration-tests docker-integration-tests/Test/compilebuild/sbt "sql/testOnly org.apache.spark.sql.jdbc.JDBCSuite"(157 tests)build/sbt "sql/testOnly org.apache.spark.sql.jdbc.PostgresDialectSuite"sql/scalastyle,sql/Test/scalastyle,docker-integration-tests/Test/scalastyleENABLE_DOCKER_INTEGRATION_TESTS=1and thedatabase images, so they were not executed locally.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: DeepSeek Harness (deepseek-v4.1-flash)