[SPARK-52825][SQL] Register existing dialects for additional URL prefixes#57455
[SPARK-52825][SQL] Register existing dialects for additional URL prefixes#57455cloud-fan wants to merge 6 commits into
Conversation
viirya
left a comment
There was a problem hiding this comment.
Nice redesign — moving the wrapper-prefix decision out of core and into an explicit user registration cleanly resolves the objection that sank #53902 (baking a specific vendor's URL scheme, and the broad jdbc:(.*:)?mysql.* regex, into the built-in dialects). Spark registers no extra prefixes by default, so compatibility stays an explicit user decision, and this lands on top of the already-reverted clean startsWith("jdbc:mysql") baseline.
I verified the behavior end to end:
- Precedence is correct.
getconsults the prefix table only whendialects.filter(_.canHandle(url))is empty, so user/built-incanHandledialects always win and a prefix registration can only fill in for URLs nothing else claims. The "registered dialect takes precedence" test pins this. - Longest prefix wins, deterministically —
registerDialectForUrlPrefixre-sorts by descending prefix length on every insert andgetDialectForUrlPrefixtakes the firststartsWithmatch, so the result is independent of registration order. - Matching stays at the protocol boundary. Prefixes are required to end with
:and matching isstartsWith, so it can't reach into the authority/path — the exact ambiguity antban raised on #53902 (jdbc:postgresql://host.mysql.com). The "does not inspect the URL authority or path" test locks that down. - Case-insensitive normalization and the
jdbc:/:validation are consistent between registration and lookup.
One non-blocking note on thread-safety. dialectsByUrlPrefix is an unsynchronized var List mutated by a read-modify-write, so concurrent writers could lose an update and a new registration isn't guaranteed visible to a concurrent reader (no @volatile). This is not a regression — it's exactly the existing contract of registerDialect/unregisterDialect on the same object, which rely on the "register once at setup, on the driver, before running queries" usage pattern; concurrent reads during get are safe because each write swaps in a fresh immutable list. If you ever want to tighten it, wrapping the register/unregister bodies (old and new) in synchronized would close both the lost-update and visibility gaps at negligible cost — but it's equally fine to leave it, since the new methods are deliberately consistent with the existing ones.
Docs and tests are thorough. LGTM.
| * Register an existing dialect for an additional JDBC URL prefix. The registration affects | ||
| * dialect lookup only; Spark still passes the original URL to the JDBC driver. Registering the | ||
| * same prefix again replaces its previous dialect. These registrations are used only when no | ||
| * registered dialect handles the URL through [[JdbcDialect.canHandle]]. |
There was a problem hiding this comment.
The scaladoc cross-reference [[JdbcDialect.canHandle]] uses dot notation; genjavadoc/unidoc converts it to {@link JdbcDialect.canHandle}, which javadoc rejects as reference not found (member links need #, not .), hard-failing the "Run / Documentation generation" check (confirmed failing on head 3f20be2). This is the sole doc reference the PR adds.
Fix: use the member-link form [[JdbcDialect#canHandle]] (the convention already used elsewhere in this file, e.g. [[java.sql.ResultSetMetaData#getColumnType]]). Trivial one-line change, but it gates merge until CI is green.
|
Thank you @cloud-fan and @viirya! I left just one more comment, otherwise LGTM |
|
I'm OK with this approach as a compromise, but I want to note that the design has some usability concerns that ideally should be addressed — particularly the reliance on internal implementation details of non-public dialect classes. Specifically: 1. Discoverability When a wrapper URL falls through to Even once a user finds the API, they need to know what string to pass to It would help to think about how users can discover and use this feature — for example, emitting a warning when falling back to 2. Implicit dependence on The prefix registration is only consulted when no registered dialect matches via |
|
I agree with @sarutak, I'm generally okay with this approach but he raises some good points that should be considered |
|
@sarutak Thanks for raising this. I agree that requiring users to call I updated the PR in a732593 to add the case-insensitive The wrapper registration now reads: JdbcDialects.registerDialectForUrlPrefix(
"jdbc:aws-wrapper:mysql:",
JdbcDialects.getBuiltInDialect("mysql"))For the precedence concern, I kept the existing behavior deliberately: normal |
a201581 to
5ff6fd8
Compare
What changes were proposed in this pull request?
This PR lets users register an existing
JdbcDialectfor an additional JDBC URL prefix. Theregistration affects dialect lookup only; Spark still passes the original URL to the JDBC driver.
It adds
JdbcDialects.registerDialectForUrlPrefixandunregisterDialectForUrlPrefix. It also addsgetBuiltInDialect, a case-insensitive lookup forthe built-in
mysql,postgresql,db2,sqlserver,derby,oracle,teradata,h2,snowflake, anddatabricksdialects. For example, a user can reuse the built-in MySQL dialectfor a known-compatible wrapper:
An unsupported built-in dialect name raises a
SparkIllegalArgumentExceptionwith theUNSUPPORTED_BUILT_IN_JDBC_DIALECTerror condition.Prefixes are case-insensitive, validated to start with
jdbc:and end with:, and consulted onlywhen no registered dialect handles the original URL through
canHandle. This preserves theprecedence of user-provided dialects. When prefixes overlap, the longest matching prefix wins.
Why are the changes needed?
Wrapper JDBC drivers can use URLs such as
jdbc:aws-wrapper:mysql://...while speaking the samedatabase dialect as the underlying MySQL or PostgreSQL driver.
Matching arbitrary nested protocols with a broad regular expression assumes that every URL of the
form
jdbc:<anything>:mysqlorjdbc:<anything>:postgresqlis compatible with the correspondingbuilt-in dialect. It can also inspect unrelated portions of the URL. Registering a specific prefix
directly to an existing dialect makes that relationship deliberate and bounded without requiring
users to inherit from or copy a built-in dialect. The named lookup makes all built-in dialects
discoverable without exposing a public collection or requiring knowledge of canonical JDBC URLs.
Spark registers no additional prefixes by default, so compatibility remains an explicit user
decision.
This follows up on the discussion in #53902.
Does this PR introduce any user-facing change?
Yes. Users can look up a built-in dialect by its documented name, and register or unregister an
existing dialect for an additional JDBC URL prefix through
JdbcDialects. Wrapper URLs remainunmatched until a user explicitly registers a dialect for their prefix.
How was this patch tested?
Added tests covering all ten built-in dialect names, case-insensitive named lookup, structured
errors for unsupported names, explicit prefix registration and replacement, case-insensitive
prefix matching, unregistration, prefix validation, longest-prefix matching, user-registered
dialect precedence, and negative cases where unregistered wrappers or database names in the URL
authority must not select a dialect.
The following commands were run:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)