yadavay-amzn commented on code in PR #57468:
URL: https://github.com/apache/spark/pull/57468#discussion_r3660217291
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkMetadataOperationSuite.scala:
##########
@@ -804,6 +804,31 @@ class SparkMetadataOperationSuite extends
HiveThriftServer2TestBase {
}
}
+ test("SPARK-57518: getSchemas for spark_catalog lists namespaces via
SupportsNamespaces " +
Review Comment:
Added a regression test with a custom `spark_catalog` override whose
`listNamespaces()` differs from `SessionCatalog.listDatabases`. It asserts the
override's namespaces show up and `default` doesn't, so it fails if the old
special-case returns.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetSchemasOperation.scala:
##########
@@ -78,34 +78,38 @@ private[hive] class SparkGetSchemasOperation(
// filtering deferred).
val resolvedCatalog = catalogManager.currentCatalog
val catalogNameValue = resolvedCatalog.name()
+ // Use SupportsNamespaces uniformly for all catalogs including the
session
+ // catalog (V2SessionCatalog implements SupportsNamespaces). This
avoids
+ // assuming that a spark_catalog override delegates to the built-in
session
+ // catalog.
+ resolvedCatalog match {
+ case nsCatalog: SupportsNamespaces =>
+ // NOTE: The DSv2 SupportsNamespaces.listNamespaces() API does not
accept a
+ // pattern argument, so filtering is applied client-side. This is
a potential
+ // performance consideration for catalogs with a very large number
of
+ // namespaces.
+ 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
+ }
+ // The global temp view database is a Spark pseudo-namespace that only
exists
+ // for the session catalog; it is not a real catalog namespace.
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))
Review Comment:
You're right, that would double-count. Fixed: I only add the `global_temp`
row when `listNamespaces()` didn't already return it, so there's at most one.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetSchemasOperation.scala:
##########
@@ -78,34 +78,38 @@ private[hive] class SparkGetSchemasOperation(
// filtering deferred).
val resolvedCatalog = catalogManager.currentCatalog
val catalogNameValue = resolvedCatalog.name()
+ // Use SupportsNamespaces uniformly for all catalogs including the
session
+ // catalog (V2SessionCatalog implements SupportsNamespaces). This
avoids
+ // assuming that a spark_catalog override delegates to the built-in
session
+ // catalog.
+ resolvedCatalog match {
+ case nsCatalog: SupportsNamespaces =>
+ // NOTE: The DSv2 SupportsNamespaces.listNamespaces() API does not
accept a
+ // pattern argument, so filtering is applied client-side. This is
a potential
+ // performance consideration for catalogs with a very large number
of
+ // namespaces.
+ 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 _ =>
Review Comment:
Added a `logWarning` there, so a catalog without `SupportsNamespaces` says
so instead of silently returning nothing.
--
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]
