maomaodev commented on PR #7718: URL: https://github.com/apache/kyuubi/pull/7718#issuecomment-5681123975
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 ...`. -- 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]
