cloud-fan commented on code in PR #56627:
URL: https://github.com/apache/spark/pull/56627#discussion_r3566799779
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetSchemasOperation.scala:
##########
@@ -71,15 +72,51 @@ private[hive] class SparkGetSchemasOperation(
try {
val schemaPattern = convertSchemaPattern(schemaName)
- catalog.listDatabases(schemaPattern).foreach { dbName =>
- rowSet.addRow(Array[AnyRef](dbName, DEFAULT_HIVE_CATALOG))
- }
-
- val globalTempViewDb = catalog.globalTempDatabase
- val databasePattern =
Pattern.compile(CLIServiceUtils.patternToRegex(schemaName))
- if (schemaName == null || schemaName.isEmpty ||
- databasePattern.matcher(globalTempViewDb).matches()) {
- rowSet.addRow(Array[AnyRef](globalTempViewDb, DEFAULT_HIVE_CATALOG))
+ if (isCatalogMetadataEnabled) {
+ // The JDBC catalogName parameter is intentionally not used for
filtering;
+ // resolution always uses the current catalog (SPARK-57518; per-catalog
+ // filtering deferred).
+ val resolvedCatalog = catalogManager.currentCatalog
+ val catalogNameValue = resolvedCatalog.name()
+ if (catalogNameValue == CatalogManager.SESSION_CATALOG_NAME) {
+ // For spark_catalog, use the V1 SessionCatalog directly
(transparent delegation)
+ catalog.listDatabases(schemaPattern).foreach { dbName =>
+ rowSet.addRow(Array[AnyRef](dbName, catalogNameValue))
+ }
+ // Global temp view database is only relevant for spark_catalog
+ val globalTempViewDb = catalog.globalTempDatabase
+ val databasePattern =
Pattern.compile(CLIServiceUtils.patternToRegex(schemaName))
+ if (schemaName == null || schemaName.isEmpty ||
+ databasePattern.matcher(globalTempViewDb).matches()) {
+ rowSet.addRow(Array[AnyRef](globalTempViewDb, catalogNameValue))
+ }
+ } else {
+ resolvedCatalog match {
+ case nsCatalog: SupportsNamespaces =>
+ val databasePattern = Pattern.compile(
+ CLIServiceUtils.patternToRegex(schemaName))
+ nsCatalog.listNamespaces().foreach { ns =>
Review Comment:
Non-blocking, and I see this is an acknowledged follow-up (design note f) —
flagging for visibility. With a DSv2 catalog as the current catalog and this
conf on, `getSchemas` here returns the DSv2 catalog's namespaces, but
`getTables`/`getColumns` still enumerate `spark_catalog`'s objects (with
empty/null `TABLE_CAT`, via `sessionCatalogTableCat`). A JDBC/BI client that
drills schema → tables then sees namespaces from one catalog and tables from
another for the same session. The `sessionCatalogTableCat` guard correctly
prevents mislabeling (nicely pinned by the negative test), so this isn't a bug
— just worth a line in the conf `doc(...)` / known-limitations so integrators
aren't surprised until the getTables/getColumns DSv2 routing follow-up lands.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetSchemasOperation.scala:
##########
@@ -71,15 +72,51 @@ private[hive] class SparkGetSchemasOperation(
try {
val schemaPattern = convertSchemaPattern(schemaName)
- catalog.listDatabases(schemaPattern).foreach { dbName =>
- rowSet.addRow(Array[AnyRef](dbName, DEFAULT_HIVE_CATALOG))
- }
-
- val globalTempViewDb = catalog.globalTempDatabase
- val databasePattern =
Pattern.compile(CLIServiceUtils.patternToRegex(schemaName))
- if (schemaName == null || schemaName.isEmpty ||
- databasePattern.matcher(globalTempViewDb).matches()) {
- rowSet.addRow(Array[AnyRef](globalTempViewDb, DEFAULT_HIVE_CATALOG))
+ if (isCatalogMetadataEnabled) {
+ // The JDBC catalogName parameter is intentionally not used for
filtering;
+ // resolution always uses the current catalog (SPARK-57518; per-catalog
+ // filtering deferred).
+ val resolvedCatalog = catalogManager.currentCatalog
+ val catalogNameValue = resolvedCatalog.name()
+ if (catalogNameValue == CatalogManager.SESSION_CATALOG_NAME) {
+ // For spark_catalog, use the V1 SessionCatalog directly
(transparent delegation)
+ catalog.listDatabases(schemaPattern).foreach { dbName =>
+ rowSet.addRow(Array[AnyRef](dbName, catalogNameValue))
+ }
+ // Global temp view database is only relevant for spark_catalog
+ val globalTempViewDb = catalog.globalTempDatabase
+ val databasePattern =
Pattern.compile(CLIServiceUtils.patternToRegex(schemaName))
+ if (schemaName == null || schemaName.isEmpty ||
+ databasePattern.matcher(globalTempViewDb).matches()) {
+ rowSet.addRow(Array[AnyRef](globalTempViewDb, catalogNameValue))
+ }
+ } else {
+ resolvedCatalog match {
+ case nsCatalog: SupportsNamespaces =>
+ val databasePattern = Pattern.compile(
+ CLIServiceUtils.patternToRegex(schemaName))
+ nsCatalog.listNamespaces().foreach { ns =>
+ // Only top-level namespaces (depth=1) are exposed as JDBC
schemas.
+ val nsName = ns.head
+ if (schemaName == null || schemaName.isEmpty ||
+ databasePattern.matcher(nsName).matches()) {
+ rowSet.addRow(Array[AnyRef](nsName, catalogNameValue))
+ }
+ }
+ case _ =>
+ // Catalog doesn't support namespaces — return empty
Review Comment:
Nit: this comment uses a non-ASCII em-dash; the repo convention is
ASCII-only in comments.
```suggestion
// Catalog doesn't support namespaces -- return empty
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2045,6 +2045,17 @@ object SQLConf {
.intConf
.createWithDefault(200)
+ val THRIFTSERVER_CATALOG_METADATA_ENABLED =
+ buildConf("spark.sql.thriftServer.catalogMetadata.enabled")
+ .doc("When true, JDBC metadata operations (getCatalogs, getSchemas,
getTables, " +
+ "getColumns) are DataSource V2 catalog-aware: getCatalogs returns all
loaded catalogs, " +
+ "and TABLE_CAT is populated with the real catalog name. When false,
the legacy behavior " +
+ "is preserved (empty TABLE_CAT and getCatalogs returns no rows).")
+ .version("5.0.0")
Review Comment:
Per Spark's branch policy, a normally-backported, conf-gated feature with no
binary-incompatibility ships first in the next open feature release branch
(`branch-4.x`, currently 4.3.0), so `.version("4.3.0")` is usually the expected
value rather than master's `5.0.0`. If this is intentionally master-only, feel
free to keep 5.0.0. (Heads-up: one other conf already on master also uses
5.0.0, so this may be fine — just confirming intent.)
--
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]
