shrirangmhalgi commented on code in PR #57468:
URL: https://github.com/apache/spark/pull/57468#discussion_r3642634618
##########
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:
nit: The `case _ =>` branch returns empty with no log. If a user's catalog
doesn't implement `SupportsNamespaces`, they get 0 schemas with no indication
why. A `logWarning` here would help diagnose "getSchemas returns nothing" bugs
- e.g., `logWarning(s"Catalog ${resolvedCatalog.name()} does not implement
SupportsNamespaces; returning empty schema list")`.
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkMetadataOperationSuite.scala:
##########
@@ -55,7 +55,7 @@ class SparkMetadataOperationSuite extends
HiveThriftServer2TestBase {
checkResult(metaData.getSchemas(null, pattern), dbs ++ dbDflts)
}
- Seq("db%", "db*") foreach { pattern =>
+ Seq("db%") foreach { pattern =>
Review Comment:
(non-blocking): The removal of `"db*"` from this assertion documents that
Hive-style glob wildcards no longer match on the V2 path. Since
`isCatalogMetadataEnabled` defaults to `true`, this is a default-on behavior
change for ThriftServer users — any client passing `*` instead of JDBC `%` will
get 0 results. Would a negative assertion (e.g.,
`checkResult(metaData.getSchemas(null, "db*"), Seq.empty)`) make this more
explicit for future readers?
--
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]
