wangzhigang1999 commented on PR #7419:
URL: https://github.com/apache/kyuubi/pull/7419#issuecomment-4341093420
## Where the patch stands now
Posting a wrap-up. Want to flag one decision up-front since it's the least
obvious from the diff and the most likely thing to want a sanity-check.
### Heads-up: how `"default"` is handled at session open
The Hive JDBC driver sends `USE_DATABASE = "default"` as a stub when the
user did not specify a database in the URL — there is no protocol-level signal
that distinguishes that stub from a genuine request for a database literally
named `default`. After the FOLLOWUP that turned `setSchema` into the
unconditional path (per your review feedback), this caused the PG and
ClickHouse `get tables` suites to fail in CI: their drivers silently accept
`setSchema("default")` (PG sets `search_path` to a non-existent schema, CH
actually has a `default` database), so the engine then scoped metadata to a
non-existent or unintended schema.
The fix is to filter `"default"` at the session entry point. The trade-off,
made explicit:
| URL form | Before this PR | After this PR |
|---|---|---|
| no database in URL | no metadata scope | no metadata scope (unchanged) |
| URL specifies a non-`default` database | no metadata scope (the original
bug) | scoped to that database ✓ |
| URL explicitly names `default` (and the backend has one) | no metadata
scope | no metadata scope (no change) |
No observable regression — the third row was already in this state before
this PR (the engine never URL-scoped at all). Users on backends with a real
`default` schema can still target it after connecting via `USE default;`, which
goes through a different code path (`newSetCurrentDatabaseOperation`). I
considered alternatives (try/catch around `setSchema`, or "verify the schema
exists before recording it") but the first is brittle on PG/CH, and the second
still does the wrong thing on ClickHouse since CH genuinely has a `default`
database.
### The rest of the patch, briefly
Three engine-side layers:
1. **Dialect API** — `JdbcDialect.setSchema(conn, schema): Unit` mirrors
`Connection#setSchema` with a leading `conn`; default forwards to the standard.
Only `MySQLDialect` overrides — calls both `setCatalog` + `setSchema` so the
database switch happens regardless of the `databaseTerm` connection property
the user configured.
2. **Session open** — invokes the dialect's `setSchema` for the URL-scoped
database (modulo the `"default"` stub above) and records it as
`effectiveDatabase`.
3. **Operation routing** — `JdbcOperationManager.newGetTablesOperation`
substitutes `effectiveDatabase` when the client sends blank or `"%"`
schemaPattern, undoing the Hive driver's `null → "%"` translation.
### Tracked separately
Migrating the dialect API from "return a SQL string" to "engine calls
`DatabaseMetaData.getXxx(...)` directly" — your point on the GetTablesOperation
thread, with cross-driver verification confirming feasibility on MySQL / PG /
CH. This PR's URL-database routing logic survives that refactor unchanged. Will
open a separate issue.
--
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]
