-
Notifications
You must be signed in to change notification settings - Fork 994
[KYUUBI #7305] Fix JDBC engine returning tables from all databases when a database is specified in the URL #7419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d01a21b
dd302b9
5c616a0
a487d97
f10df51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,8 @@ package org.apache.kyuubi.engine.jdbc.operation | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.commons.lang3.StringUtils | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.kyuubi.KyuubiSQLException | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.kyuubi.config.KyuubiConf | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.kyuubi.config.KyuubiConf.{ENGINE_JDBC_FETCH_SIZE, ENGINE_JDBC_OPERATION_INCREMENTAL_COLLECT} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall the details of the initial code of this part, but the API looks problematic. the Hive Thrift GetTablesOperation API is derived from the JDBC API, and that's why they look almost identical, for JDBC engine, such a call should directly map to the JDBC API instead of requesting a SQL (if you take a look at the JDBC driver implementation, you may find some impls are non-SQL-based), the current design complicates the whole thing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Cross-driver behavior of each API:
Things that hold across all three drivers:
So the JDBC standard API really does cover every operation the dialect models today, across very different DBMS families. Concretely, this means the 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:
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if we do that first, do we still need this patch?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, the JDBC API Javadocs define behavior in a very clear way, it should not throw
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, we MUST avoid copying the code from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes — the bug is the Hive JDBC client converting So the engine still has to substitute the URL-scoped database into the call when the client sends What the refactor does subsume: the two Happy either way on ordering — bug fix first then refactor, or refactor first and a smaller patch on top.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed, and thanks for catching this early. I've rewritten all my prior PR comments and the in-tree comment in |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| val normalizedConf = session.asInstanceOf[JdbcSessionImpl].normalizedConf | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| val fetchSize = normalizedConf.get(ENGINE_JDBC_FETCH_SIZE.key).map(_.toInt) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| .getOrElse(session.sessionManager.getConf.get(ENGINE_JDBC_FETCH_SIZE)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.