[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:

   • IcebergMetadataTests#get tables — exercises both spark_catalog (hive-backed session catalog) and hadoop_prod (hadoop-backed v2 catalog), now asserting TABLE_SCHEM/TABLE_CAT and dropping namespaces in finally.
   • IcebergMetadataTests#get tables and views — custom session catalog view detection, verifying tableTypes filtering (TABLE/VIEW).
   • IcebergMetadataTests#get tables with ignoreTableProperties — covers all three behaviors of ignoreTableProperties (skip view probe, skip tableTypes filter, skip per-table property loading).
   • SparkCatalogUtilsSuite#getSchemas lists DSv2-only namespace — covers the custom session catalog GetSchemas path via a test-only DummySessionCatalog exposing a namespace the v1 catalog cannot see.
   • 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 -u "jdbc:hive2://<kyuubi-host>:<port>/" -n <user> -e "!tables". This previously threw ClassNotFoundException: org.apache.iceberg.mr.hive.HiveIcebergStorageHandler, and now returns the table as TABLE and v1/v2 views as VIEW.

Limitations

• Non-session v2 catalogs report tables only (all rows typed TABLE); the tableTypes filter is not applied to them, so a VIEW-only request still returns those tables typed TABLE (no VIEW rows are produced by the catalog itself). DSv2 ViewCatalog is still immature, so covering v2 tables only is intentional for this PR.
• The fix guarantees listing for custom-catalog tables it can load; tables delegated back to v1 (e.g. non-Iceberg tables under Iceberg's session catalog) still follow the v1 path.

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.

[maomaodev]

Thanks for the thorough review. All blockers and majors are addressed, code, tests, and PR metadata are now aligned. Point-by-point:

1. [Blocker] Local temp views leak — Fixed. Identifiers are filtered by t.namespace().nonEmpty before typing/loading, matching master's includeLocalTempViews = false.

2. [Blocker] Unguarded loadTable — Fixed. Wrapped in Try and degrade REMARKS to "" on failure.

3. [Blocker] View probe keys by outer db — Fixed. listViews results are filtered by .filter(_.database.isDefined).

4. [Major] getSchemas backtick-quoted — Fixed. For a custom session catalog, getSchemas now lists namespaces through DSv2 (getSchemasWithPattern(catalog, schemaPattern, quote = false)) with quoting disabled, so a DB like 2026_db is returned raw and remains matchable by its own pattern, matching master's raw-name behavior.

5. [Major] Non-exhaustive listAllNamespaces — Fixed. Added a case _ => Array.empty fallback, so a TableCatalog-only custom session catalog no longer throws MatchError in getSchemas.

6. [Major] listViews requireDbExists — Fixed. The per-namespace probe is guarded with sessionCatalog.databaseExists(db).

7. [Major] hasCustomSessionCatalog case-sensitivity — Fixed. Uses equalsIgnoreCase.

8. [Major] tableTypes on ordinary v2 catalogs — Fixed. Dropped the filter, restoring master behavior (non-session v2 catalogs report tables only, typed TABLE). Limitation documented in the PR description.

9. [Major] Scope/claim — Addressed. The listing is now independent of per-catalog loading (Try-guarded loadTable + databaseExists probe), and the claim is narrowed in the PR description.

10. [Major] TABLE_CAT inconsistency — Fixed. All row producers use catalog.name().

11. [Test] Delta get tables and views — Removed the non-discriminating Delta case; the Iceberg suite exercises the custom-session-catalog path and fails without the fix.

12. [Test] GetSchemas coverage — Added SparkCatalogUtilsSuite#getSchemas lists DSv2-only namespace. It wires a test-only DummySessionCatalog (a DelegatingCatalogExtension exposing a namespace the v1 catalog cannot see) and asserts getSchemas lists it through DSv2 with raw (unquoted) names — the test fails without the hasCustomSessionCatalog detection and quote = false.

13. [Test] Un-dropped namespaces — Fixed. All namespace-creating tests now drop namespaces in finally.

14. [Test] ignoreTableProperties coverage — Added get tables with ignoreTableProperties, covering all three behaviors (view probe, tableTypes filter, loadTable).

15. [Minor] TABLE_SCHEM/TABLE_CAT + docstring + name()/name — Fixed. Assertions added, overstating docstring removed, unified to catalog.name().

16. [Minor] HMS container duplication — Fixed. Shared DOCKER_IMAGE_NAME/EXPOSED_HMS_PORT, added hmsThriftUris null guard, and added startup timeout.

17. [Hygiene] Commit tag / description — Commit retagged to #7673; PR description rewritten to reflect the final diff, and the beeline command is now written out in full instead of beeline ....