[KYUUBI #7673] Fix thrift GetSchemas/GetTables with custom session catalog

[maomaodev]

Why are the changes needed?

The thrift GetSchemas/GetTables operations assumed spark_catalog is always the builtin V1 session catalog and listed databases/tables via spark.sessionState.catalog. When a custom session catalog is configured, e.g.

spark.sql.catalog.spark_catalog=org.apache.iceberg.spark.SparkSessionCatalog

these operations bypass the DSv2 catalog APIs (SupportsNamespaces/TableCatalog) and the V1 session catalog path cannot even parse the V2 table metadata the custom catalog persists in HMS - it may return wrong results, or throw and fail the whole request. #7673 is a real-world example: GetTables fails with ClassNotFoundException: org.apache.iceberg.mr.hive.HiveIcebergSerDe when the V1 HiveExternalCatalog tries to load tables registered by Iceberg with storage_handler = org.apache.iceberg.mr.hive.HiveIcebergStorageHandler.

How was this patch tested?

1. Added test coverage: DeltaMetadataTests#get tables and views (v1 view path), IcebergMetadataTests#get tables and views (v2 view path), and IcebergMetadataTests#get tables (both spark_catalog and hadoop_prod). A testcontainers-based WithSimpleHMSContainer is wired into SparkIcebergOperationSuite and IcebergOperationSuite so these run against a real Hive metastore (catalogImplementation=hive).
2. Manually verified against Iceberg 1.10.1 (which removed iceberg-hive-runtime): beeline ... -e "!tables" previously threw ClassNotFoundException: org.apache.iceberg.mr.hive.HiveIcebergStorageHandler, and now returns the table as TABLE and v1/v2 views as VIEW.

Was this patch assisted by generative AI tooling?

Assisted-by: DeepSeek-V4-Pro

[pan3793]

Review of #7718 (head 8ffa541). Verdict: request changes. The #7673 diagnosis and the Iceberg fix mechanism are verified real; the listing refactor introduces the regressions below in the same configurations the PR targets.

1. [Blocker, regression] Local temp views leak into the new v2 listing. catalog.listTables(ns) (SparkCatalogUtils.scala:202-204) reaches V2SessionCatalog.listTables -> SessionCatalog.listTables(db) with includeLocalTempViews = true, so temp views come back as Identifier.of(Array(), name); they are typed TABLE with TABLE_SCHEM="" (:217-232) and getTempViews (:350-359) emits the same view a second time as VIEW. Master used includeLocalTempViews = false here. Fix: drop identifiers with an empty namespace before typing/loading.

2. [Blocker, regression] catalog.loadTable(ident) at :229 is unguarded and runs per row when ignoreTableProperties=false (the default), so the namespace-less identifier from (1) throws (requiresSinglePartNamespace/NoSuchTable) and fails the whole GetTables; any unloadable, dropped, or corrupt table does the same. It also forces per-table format I/O (Delta _delta_log, Iceberg metadata JSON) where master did one bulk HMS call. Fix: wrap in Try and degrade REMARKS to "" (or keep the session-catalog listing load-free).

3. [Blocker, regression] The view probe keys listViews results by the outer db (:211-215) while SessionCatalog.listViews also returns local temp views, so a temp view v marks permanent table db.v as VIEW and removes it under tableTypes={TABLE}. Fix: only keep TableIdentifiers with a database.

4. [Major, regression] getSchemas("spark_catalog", ...) with a custom session catalog now returns backtick-quoted namespaces and matches the pattern against the quoted form (:126, :170-173); a DB like 2024_db is returned as `2024_db` and no longer matchable by its own pattern. Master returned raw v1 names.

5. [Major, regression] listAllNamespaces is a non-exhaustive match (:153-163) and is now reachable for the session catalog, so a TableCatalog-only session catalog throws MatchError in GetSchemas where master returned v1 rows.

6. [Major, new failure mode] The per-namespace spark.sessionState.catalog.listViews(db, ...) probe calls requireDbExists, so a custom session catalog whose namespaces are not v1-backed now fails the whole GetTables with NoSuchDatabaseException. Guard with databaseExists or catch and skip the probe for that namespace.

7. [Major] hasCustomSessionCatalog (:106-109) compares case-sensitively; Spark 4.x normalizes BUILTIN to builtin, so such a value is treated as custom. Use equalsIgnoreCase (mirrors V2SessionCatalog.hasCustomSessionCatalog).

8. [Major, behavior change] tableTypes is now applied to ordinary v2 catalogs (:225) while view detection exists only for the session catalog, so getTables(<v2 catalog>, ..., Array("VIEW")) goes from all-rows-typed-TABLE to empty. DSv2 ViewCatalog is still immature, so covering v2 tables only is fine for this PR - please just document the limitation (non-session v2 catalogs report tables only, VIEW-only filters return empty) and drop the filter or add a test pinning the empty-result behavior.

9. [Major, scope/claim] The fix guarantees only "the custom catalog can load the table". Tables delegated back to v1 (e.g. DeltaCatalog + catalogImplementation=hive, or non-Iceberg tables under Iceberg's session catalog) still hit HiveExternalCatalog/HiveClientImpl.getStorageHandler and the same ClassNotFoundException. Narrow the claim, or make the listing independent of per-catalog loading.

10. [Major] TABLE_CAT is inconsistent within one result: catalog.name() at :231 vs the raw request string in getTempViews (:357) and getColumnsByCatalog, so getTables(null, ...) mixes "spark_catalog" with null. Align all row producers and tighten the assertions.

11. [Test] DeltaMetadataTests#get tables and views exercises the new path (DeltaCatalog is a custom session catalog) but passes on master, so it does not discriminate the fix. Add a case that fails without the change, or drop it.

12. [Test] Nothing covers GetSchemas: reverting only the getSchemas change keeps the suites green, because the test session catalogs delegate namespaces to the v1 catalog. Add a test-only CatalogExtension exposing a namespace the v1 catalog cannot see.

13. [Test] IcebergMetadataTests creates spark_catalog.<db> namespaces and never drops them (withDatabases only drops unqualified names), making the exact-count get schemas assertion order-dependent. Drop them in the finally blocks.

14. [Test] kyuubi.operation.getTables.ignoreTableProperties has no coverage repo-wide; the new path has three distinct behaviors for it (:208, :225, :228). Add a test.

15. [Minor] The new tests never assert TABLE_SCHEM/TABLE_CAT; the docstring at :186-191 overstates ("returns both v1 and v2 views", "never loads the Hive storage handler") - it only covers views registered in the v1 catalog; catalog.name() and catalog.name are mixed in one file.

16. [Minor] The new WithSimpleHMSContainer duplicates WithSecuredHMSContainer (drop the unused env, consider sharing the container def), hmsThriftUris has no null guard, and HostPortWaitStrategy has no startup-timeout margin.

17. [Hygiene] Commit 80255d65e is tagged #7064 while the PR head/title use #7673, and the description does not describe the final diff or list the commands run. Please update the description for the final state.