[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?
2 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.