sarutak commented on PR #53902:
URL: https://github.com/apache/spark/pull/53902#issuecomment-4850622875
I agree that making it easy to reuse builtin dialect code is a valuable
goal, and the DelegatingDialect idea is interesting. However, I think it needs
more careful design consideration. For example, if a `TiDBDialect` using this
pattern matches `jdbc:mysql://`, it would either create an `AggregatedDialect`
with `MySQLDialect` (affecting all MySQL connections) or, without
`AggregatedDialect`, unconditionally override the builtin for all MySQL URLs.
The fundamental issue is that `canHandle(url: String)` cannot distinguish
databases that share the same URL prefix. Solving this properly likely requires
rethinking the dialect resolution architecture (e.g., explicit dialect option,
post-connection metadata detection), which is well beyond the scope of this PR.
Regarding Option A (regex): the concern that "we don't really know if it's
correct" for arbitrary `jdbc:(.*:)?mysql` URLs
is fair, but `startsWith("jdbc:mysql")` already matches `jdbc:mysqlx` (X
DevAPI) and `jdbc:mysql:replication:`. This is the same category of risk that
already exists. The regex doesn't introduce a new class of problem.
Given this, I'd still lean toward a simple `canHandle` fix in this PR
(either the original `||` approach or regex), and leave the broader dialect
extensibility discussion to a dedicated JIRA/PR.
--
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]
