wangzhigang1999 commented on code in PR #7419:
URL: https://github.com/apache/kyuubi/pull/7419#discussion_r3154275247
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/JdbcOperationManager.scala:
##########
@@ -91,7 +93,12 @@ class JdbcOperationManager(conf: KyuubiConf) extends
OperationManager("JdbcOpera
schemaName: String,
tableName: String,
tableTypes: util.List[String]): Operation = {
- val query = dialect.getTablesQuery(catalogName, schemaName, tableName,
tableTypes)
+ val effectiveSchema = if (StringUtils.isBlank(schemaName) || schemaName ==
"%") {
+ session.asInstanceOf[JdbcSessionImpl].effectiveDatabase.orNull
+ } else {
+ schemaName
+ }
+ val query = dialect.getTablesQuery(catalogName, effectiveSchema,
tableName, tableTypes)
Review Comment:
Thanks for raising this — you're right, and I want to confirm I took it
seriously rather than hand-wave it away. To make sure I understood, I ran the
full set of `DatabaseMetaData` calls against three different drivers
(mysql-connector-j 8.4.0, postgresql 42.7.2, clickhouse-jdbc 0.6.5) using a
small Java probe and docker containers. Test fixtures were tailored to each
backend (MySQL/PG had PK + FK + a function; ClickHouse used MergeTree which has
no native PK/FK).
Cross-driver behavior of each API:
| API | MySQL (`databaseTerm=CATALOG`) | MySQL (`databaseTerm=SCHEMA`) |
PostgreSQL | ClickHouse |
|---|---|---|---|---|
| `getCatalogs()` | ✓ implemented | ⚠ empty (driver `databaseTerm` quirk) |
✓ implemented | ✓ implemented |
| `getSchemas()` | ⚠ empty (driver `databaseTerm` quirk) | ✓ implemented | ✓
implemented | — model has no schema layer |
| `getTableTypes()` | ✓ | ✓ | ✓ | ✓ |
| `getTables(...)` | ✓ | ✓ | ✓ | ✓ |
| `getColumns(...)` | ✓ | ✓ | ✓ | ✓ |
| `getPrimaryKeys` | ✓ | ✓ | ✓ | — MergeTree has no PK |
| `getImported / Exported / CrossReference` | ✓ | ✓ | ✓ | — no FK in CK |
| `getFunctions(...)` | ✓ | ✓ | ✓ | (no fixture created) |
| `getTypeInfo()` | ✓ | ✓ | ✓ | ✓ |
Things that hold across all three drivers:
- **No `SQLFeatureNotSupportedException` anywhere** — every JDBC-standard
metadata method is implemented and returns a `ResultSet` rather than throwing,
even when the data model doesn't populate it.
- **Column names follow the JDBC spec** (`TABLE_CAT`, `TABLE_SCHEM`,
`TABLE_NAME`, `COLUMN_NAME`, `DATA_TYPE`, `KEY_SEQ`, `PK_NAME`, ...) — which is
exactly what `GetTablesOperation` / `GetColumnsOperation` / etc. on the Hive
Thrift side expect, since the Thrift API was derived from the JDBC API in the
first place.
- **Driver-specific quirks are minor and well-defined** — `databaseTerm`
gating is unique to mysql-connector-j; PG returns column labels in lowercase
while MySQL/CK use uppercase (JDBC spec says case-insensitive, but engine-side
string comparisons would need to match).
So the JDBC standard API really does cover every operation the dialect
models today, across very different DBMS families. Concretely, this means the
`JdbcDialect.getXxxQuery` family of methods could be deleted in favor of the
engine calling `connection.getMetaData.getXxx(...)` directly — and dialects
whose driver implements metadata via a non-SQL path (Avatica, etc.) would
automatically be used the way they were designed to.
The few wrinkles I hit are all driver-side and resolvable in a few lines of
engine-level adapter code, not by reaching for SQL:
- `getCatalogs` / `getSchemas` are mutually exclusive on mysql-connector-j
(gated by `databaseTerm`) — handled by checking the property and routing, or by
calling both and taking whichever is non-empty.
- `DATA_TYPE` is `int` in the JDBC spec but the current SQL returns a string
— `java.sql.Types`-to-name mapping is one helper.
- The current SQL returns extra MySQL-specific columns (`ENGINE`,
`TABLE_ROWS`, ...) but the Hive Thrift `GetTables` result schema is fixed to
JDBC-standard columns, so those extras are already dropped during serialization
today and aren't reaching clients — no real loss.
I'd like to keep this PR scoped to the URL-database bug since that's what it
set out to fix, and migrating the dialect API to call `DatabaseMetaData`
directly will touch every dialect, the operation manager, and every metadata
test — not something I'd want to fold into a bug fix. Happy to open a follow-up
issue tracking the migration if you agree with that split. WDYT?
--
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]
