sarutak commented on PR #53902:
URL: https://github.com/apache/spark/pull/53902#issuecomment-4848067904
> Design / architecture (1)
MySQLDialect.scala:41 / PostgresDialect.scala:45: jdbc:aws-wrapper:
hardcodes a single vendor's library URL scheme into core — see inline
jdbc:aws-wrapper: is not a JDBC standard; it is the URL scheme of one vendor
product (AWS aws-advanced-jdbc-wrapper, for Aurora). The wrapper deliberately
speaks standard MySQL/PostgreSQL underneath, so — as the PR description notes —
this adds no dialect behavior; it only bakes a third-party library's URL prefix
into core matching. The Snowflake/Databricks dialects aren't an analogous
precedent (those are genuinely distinct SQL backends), and accepting a vendor
prefix here has no principled stopping point: every other connection
wrapper/proxy (p6spy, other clouds' Aurora-style wrappers) gains an equal claim.
>
> The genuine friction this PR works around is that
MySQLDialect/PostgresDialect are private case classes, so a wrapper user can't
subclass them to override canHandle. The Spark-idiomatic fix for that is to
expose the builtin dialects as undocumented advanced APIs — drop private and
annotate @DeveloperApi (the base JdbcDialect and JdbcDialects.registerDialect
are already @DeveloperApi). A user can then write class AwsMySQLDialect extends
MySQLDialect, override canHandle to match only jdbc:aws-wrapper:mysql, and
register it via JdbcDialects.registerDialect (user dialects are tried first).
The wrapper URL resolves to their subclass — which inherits all real dialect
behavior — jdbc:mysql stays on the builtin, there's no AggregatedDialect
overlap, and no vendor string enters core. Would you be open to that shape
instead?
I've explored the proposed approach further and have some concerns:
**1. case class inheritance is problematic**
All existing Dialects are `private case class`. Inheriting from a case class
is deprecated in Scala 2 and prohibited in Scala 3. This pattern doesn't exist
anywhere in the Spark codebase.
**2. Converting to regular classes has a large blast radius**
To make subclassing viable, we'd need to convert case classes to regular
classes, which requires manual `equals`/`hashCode` implementations (used by
`registerDialect`/`unregisterDialect`), test modifications (`===
MySQLDialect()` assertions), and potentially affects all 10 Dialect classes for
consistency. This is a significant refactoring for a 2-line fix.
**3. The DeveloperApi approach doesn't actually serve the broader need, and
could introduce problems**
I considered the use case of users wanting to extend MySQLDialect for
databases like TiDB or CockroachDB (which are MySQL/PostgreSQL-compatible but
have dialect differences). However, since these databases use the same URL
prefix (`jdbc:mysql://`, `jdbc:postgresql://`), a subclass's `canHandle` cannot
distinguish them from the original. Both match, resulting in
`AggregatedDialect` rather than the intended custom Dialect being selected
alone. Worse, the custom Dialect would also match **other** connections using
the same prefix, potentially breaking unrelated MySQL/PostgreSQL connections
with incorrect type mappings.
So the DeveloperApi approach:
- Only works for the case where the URL prefix is **different** (like
aws-wrapper) but that's better solved with a simpler `canHandle` fix
- Does **not** work for the case where the URL prefix is the **same** (like
TiDB/CockroachDB) which is where Dialect extension is actually needed
- Could lead users to create subclasses that inadvertently break other
connections via AggregatedDialect
We should avoid exposing a problematic API to solve a small problem. The
discussion about making Dialects properly extensible (e.g., explicit dialect
option, post-connection detection) is valuable but is a separate concern that
would need a different mechanism. It shouldn't block this PR.
### Compromise proposals
If the concern is specifically about vendor-specific strings in
`MySQLDialect`/`PostgresDialect`, here are two alternatives:
**Option A: Regex matching (preferred)**
Instead of hardcoding `"jdbc:aws-wrapper:mysql"`, use a pattern that
generically accepts any wrapper prefix between `jdbc:` and the database name:
```scala
override def canHandle(url: String): Boolean =
url.toLowerCase(Locale.ROOT).matches("jdbc:(.*:)?mysql.*")
```
This:
- Avoids any vendor-specific string in core
- Covers all known wrapper patterns (aws-wrapper, p6spy, log4jdbc) without
enumerating them
- Eliminates the "no principled stopping point" concern — it's one generic
rule, not an open-ended list
- Known URL patterns for MySQL (`jdbc:mysql:`, `jdbc:mysql+srv:`,
`jdbc:mysql:replication:`, `jdbc:mysql:loadbalance:`, `jdbc:mysqlx:`,
`jdbc:aws-wrapper:mysql:`, `jdbc:p6spy:mysql:`) are all covered
- For other Dialects (Oracle, DB2, etc.), no wrapper usage has been
reported, so we don't propose changes to those in this PR
**Option B: Trait extraction**
Extract the shared logic into a trait and create a separate
`AuroraMySQLDialect` class:
```scala
trait MySQLDialectBase extends JdbcDialect with SQLConfHelper with
NoLegacyJDBCError {
// all existing logic
}
private case class MySQLDialect() extends MySQLDialectBase {
override def canHandle(url: String): Boolean =
url.toLowerCase(Locale.ROOT).startsWith("jdbc:mysql")
}
private case class AuroraMySQLDialect() extends MySQLDialectBase {
override def canHandle(url: String): Boolean =
url.toLowerCase(Locale.ROOT).startsWith("jdbc:aws-wrapper:mysql")
}
```
This keeps vendor strings in a dedicated class (similar to how
`DatabricksDialect` contains `"jdbc:databricks"`), though admittedly it creates
a class with no dialect differences. Only a `canHandle` override.
I'd lean toward Option A as it's simpler and more principled, but either
approach addresses the core concern of keeping vendor strings out of
`MySQLDialect`.
--
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]
