[SPARK-57518][SQL][FOLLOWUP] List ThriftServer schemas via SupportsNamespaces instead of special-casing spark_catalog#57468
Conversation
…upportsNamespaces instead of special-casing spark_catalog
|
@pan3793 here's the follow-up we discussed. It lists schemas uniformly via the current catalog's |
| // 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. |
There was a problem hiding this comment.
@yadavay-amzn I think we can accept this for now, as users still have a backdoor (isCatalogMetadataEnabled) to go back to the old impl.
@cloud-fan for the long term, do we want to extend the V2 API to support predication pushdown on listing ns/table/func/col? and the approach can also be either simple - just accept a string pattern like the current ExternalCatalog, or extendable - just like SupportsPushDownFilters
| val databasePattern = Pattern.compile(CLIServiceUtils.patternToRegex(schemaName)) | ||
| if (schemaName == null || schemaName.isEmpty || | ||
| databasePattern.matcher(globalTempViewDb).matches()) { | ||
| rowSet.addRow(Array[AnyRef](globalTempViewDb, catalogNameValue)) |
There was a problem hiding this comment.
What if the catalog has a real database named globalTempViewDb?
shrirangmhalgi
left a comment
shrirangmhalgi
left a comment
There was a problem hiding this comment.
Traced the pattern-matching chain: V1 catalog.listDatabases(schemaPattern) → StringUtils.filterPattern ((?i) + Hive glob *) vs V2 CLIServiceUtils.patternToRegex → Pattern.compile (JDBC %/_ only, no (?i)).
Verification
V2SessionCatalog.listNamespaces()delegates tocatalog.listDatabases()(noglobal_temp) - separate append is neededlistNamespaces()returnsArray[Array[String]]- materialized before iteration, no partial-result riskisCatalogMetadataEnableddefaultstrue(4.3.0+) - this is the default path, not opt-increateDatabaseblocksglobal_tempname for built-in SessionCatalog - duplicate risk is custom CatalogExtension only
Non Blocking Suggestions (2)
- Test file -
db*removal deserves a negative assertion - see inline - Production -
case _ =>silent empty - see inline
| rowSet.addRow(Array[AnyRef](nsName, catalogNameValue)) | ||
| } | ||
| } | ||
| case _ => |
There was a problem hiding this 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").
| } | ||
|
|
||
| Seq("db%", "db*") foreach { pattern => | ||
| Seq("db%") foreach { pattern => |
There was a problem hiding this 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?
3 participants
What changes were proposed in this pull request?
Follow-up to #56627 (SPARK-57518), addressing review feedback.
SparkGetSchemasOperationspecial-cased the session catalog: when the current catalog wasspark_catalog, it listed databases via the V1SessionCatalog.listDatabases, and only used the DSv2SupportsNamespaces.listNamespaces()path for other catalogs. That assumed aspark_catalogoverride (e.g. a customCatalogExtension) delegates its namespace listing to the built-in session catalog, which is not guaranteed.This change lists schemas uniformly through the current catalog's
SupportsNamespaces.listNamespaces()for all catalogs, and drops thespark_catalogspecial-case. The built-inV2SessionCatalogalso implementsSupportsNamespaces, so the default session catalog keeps working, and an overriddenspark_catalognow correctly reports its own namespaces. Theglobal_tempdatabase is retained as a special case for the session catalog only, since it is a Spark pseudo-namespace rather than a real catalog namespace.Why are the changes needed?
The previous approach could report the wrong schemas for a
spark_catalogoverride that does not delegate namespace listing to the built-in session catalog.Does this PR introduce any user-facing change?
Behavior change on the DSv2 metadata path only: schema-name matching now uses JDBC pattern semantics (
%and_), consistent withDatabaseMetaData.getSchemas. Hive-style glob (*) is not a JDBC wildcard and is no longer matched on this path.Performance consideration
SupportsNamespaces.listNamespaces()has no schema-pattern argument (unlike V1listDatabases(pattern)), so the schema pattern is applied client-side. For a catalog with many namespaces this lists all top-level namespaces and then filters, rather than pushing the pattern down. The current DSv2 API does not support pushing the pattern down; if this becomes a concern, a DSv2 API enhancement to accept a pattern would be a separate improvement. Raising it here for discussion.How was this patch tested?
SparkMetadataOperationSuite: verifiesspark_cataloglists its namespaces viaSupportsNamespacesand thatglobal_tempstill appears. Existing getSchemas coverage passes.SparkGetTablesOperation/SparkGetColumnsOperationdo not have this special-case and are unchanged.Was this patch authored or co-authored using generative AI tooling?
Authored with assistance by Claude Opus 4.8.