[improvement](catalog) avoid loading every table for SHOW TABLES on external catalogs - #66080
Conversation
…xternal catalogs ### What problem does this PR solve? Issue Number: close apache#66079 Problem Summary: `SHOW TABLES` on an external catalog iterated `dbIf.getTables()`, which eagerly initializes every table via the meta cache (one `getTableNullable()` -> remote metadata load per table). For catalogs with many tables this turned a cheap name listing into N remote loads. This adds a fast path for the common case only -- a non-verbose `SHOW TABLES` on an external catalog -- that lists names via `dbIf.getTableNamesOrEmptyWithLock()` without initializing any table. The per-table `SHOW` privilege filter is preserved (it is name-based and needs no table load), so no table name is leaked to a user who lacks `SHOW` on it. Everything else keeps the original `getTables()` loop unchanged: - Internal-catalog `SHOW TABLES` / `SHOW FULL TABLES`. - External-catalog `SHOW FULL TABLES` (verbose): it still needs `getMysqlType()` and the storage-format columns, so it takes the original path and its 4-column output (`Table_type`, `Storage_format`, `Inverted_index_storage_format`) is unchanged. - `SHOW VIEWS` (needs `getEngine()` to filter views) and `SHOW STREAMS`. ### Release note None (performance optimization; `SHOW TABLES` output for accessible tables is unchanged, `SHOW FULL TABLES` / `SHOW VIEWS` are unaffected). ### Check List (For Author) - Test: - Manual test on a live cluster with an HMS catalog: `SHOW TABLES` and `SHOW TABLES LIKE` list names via the fast path; `SHOW FULL TABLES` returns the correct 4 columns (name, `Table_type`, `Storage_format`, `Inverted_index_storage_format`) with no column mismatch. - Ran existing `ShowTableCommandTest`; FE build (`build.sh --fe`) + checkstyle green. - No new unit test: exercising the external-catalog branch needs an ExternalCatalog with a working `getTableNamesOrEmptyWithLock()`, which is not available in the FE unit-test harness; a portable regression `.out` needs the docker HMS test environment. - Behavior changed: No material change (a non-loadable external table's name is now listed by `SHOW TABLES` where the old path silently omitted it on load failure, which is arguably more correct). - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29392 ms |
TPC-DS: Total hot run time: 177307 ms |
ClickBench: Total hot run time: 25.25 s |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
…sensitive name conflicts ### What problem does this PR solve? Problem Summary: The fast path for non-verbose SHOW TABLES on external catalogs used dbIf.getTableNamesOrEmptyWithLock(), which is a defensive wrapper that catches all exceptions and returns an empty set. This swallowed the case-insensitive name-conflict / meta_names_mapping exception (FOUND_CONFLICTING) that is raised while listing table names in the meta cache. The previous dbIf.getTables() path called getTableNamesWithLock() directly and propagated that exception, so SHOW TABLES correctly reported conflicting remote table names. As a result, external regression tests test_conflict_name and test_meta_names_mapping failed: they expect SHOW TABLES to throw "Found conflicting table names under case-insensitive conditions ... Please use meta_names_mapping to handle name mapping." but the exception was silently swallowed and an empty result was returned. Fix: use getTableNamesWithLock() instead of getTableNamesOrEmptyWithLock(). This keeps the performance benefit of the fast path (list names only, one remote list call, no per-table metadata load) while restoring the name-conflict detection contract that matches the old getTables() behavior. ### Release note None ### Check List (For Author) - Test: Regression test (external_table_p0/lower_case/test_conflict_name, test_meta_names_mapping) - Behavior changed: No - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 29960 ms |
TPC-DS: Total hot run time: 177540 ms |
ClickBench: Total hot run time: 24.94 s |
|
PR approved by at least one committer and no changes requested. |
…al catalogs Cover the SHOW TABLES fast path added in apache#66080: - plain SHOW TABLES on an external catalog lists names via getTableNamesWithLock() and never initializes table objects - LIKE pattern and SHOW privilege filters still apply on the fast path - name-listing failures (case-insensitive name conflicts) propagate instead of being swallowed into an empty result - SHOW FULL TABLES / SHOW VIEWS / internal catalog keep iterating table objects (temporary tables still hidden)
|
Unit tests are ready for your review: Baymine#1 — 7 cases covering the fast path, LIKE/privilege filters, conflict-error propagation, and the unchanged verbose/view/internal paths. All pass against your branch; merge it if you'd like them included here. |
TPC-H: Total hot run time: 29359 ms |
TPC-DS: Total hot run time: 166186 ms |
ClickBench: Total hot run time: 27.53 s |
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by at least one committer and no changes requested. |
…dTest Address review feedback: keep one test class per tested class (Google Java Style) and reuse the existing ShowTableCommandMocks helper instead of a separate ShowTableCommandExternalTest. The listsNamesOnly, privilege-filter, SHOW FULL TABLES, SHOW VIEWS and internal-catalog cases duplicate the call-count coverage added in cad2f74 and are dropped. Retained new coverage: - LIKE pattern filter applies on the names-only fast path and getTables() is never called - name-listing failures (case-insensitive name conflicts) raised by getTableNamesWithLock() propagate instead of being swallowed into an empty result Verified: mvn -pl fe-core -am test -Dtest=ShowTableCommandTest, 8/8 pass.
[test](catalog) Add unit tests for SHOW TABLES name listing on external catalogs
|
run buildall |
TPC-H: Total hot run time: 29645 ms |
TPC-DS: Total hot run time: 158418 ms |
ClickBench: Total hot run time: 23.69 s |
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by at least one committer and no changes requested. |
…xternal catalogs (apache#66080) ### What problem does this PR solve? Issue Number: close apache#66079 Problem Summary: `SHOW TABLES` on an external catalog iterated `dbIf.getTables()`, which eagerly initializes every table via the meta cache (one `getTableNullable()` -> remote metadata load per table). For catalogs with many tables this turned a cheap name listing into N remote loads. This adds a fast path for the common case only -- a non-verbose `SHOW TABLES` on an external catalog -- that lists names via `dbIf.getTableNamesOrEmptyWithLock()` without initializing any table. The per-table `SHOW` privilege filter is preserved (it is name-based and needs no table load), so no table name is leaked to a user who lacks `SHOW` on it. Everything else keeps the original `getTables()` loop unchanged: - Internal-catalog `SHOW TABLES` / `SHOW FULL TABLES`. - External-catalog `SHOW FULL TABLES` (verbose): it still needs `getMysqlType()` and the storage-format columns, so it takes the original path and its 4-column output (`Table_type`, `Storage_format`, `Inverted_index_storage_format`) is unchanged. - `SHOW VIEWS` (needs `getEngine()` to filter views) and `SHOW STREAMS`. ### Release note None (performance optimization; `SHOW TABLES` output for accessible tables is unchanged, `SHOW FULL TABLES` / `SHOW VIEWS` are unaffected). ### Check List (For Author) - Test: - Manual test on a live cluster with an HMS catalog: `SHOW TABLES` and `SHOW TABLES LIKE` list names via the fast path; `SHOW FULL TABLES` returns the correct 4 columns (name, `Table_type`, `Storage_format`, `Inverted_index_storage_format`) with no column mismatch. - Ran existing `ShowTableCommandTest` (2 passed); FE build (`build.sh --fe`) + checkstyle green. - No new unit test: exercising the external-catalog branch needs an ExternalCatalog with a working `getTableNamesOrEmptyWithLock()`, which is not available in the FE unit-test harness; a portable regression `.out` needs the docker HMS test environment. - Behavior changed: No material change (a non-loadable external table's name is now listed by `SHOW TABLES` where the old path silently omitted it on load failure, which is arguably more correct). - Does this need documentation: No --------- Co-authored-by: lbs <lucian1412@outlook.com>
What problem does this PR solve?
Issue Number: close #66079
Problem Summary:
SHOW TABLESon an external catalog iterateddbIf.getTables(), whicheagerly initializes every table via the meta cache (one
getTableNullable()-> remote metadata load per table). For catalogs with many tables this turned
a cheap name listing into N remote loads.
This adds a fast path for the common case only -- a non-verbose
SHOW TABLESon an external catalog -- that lists names via
dbIf.getTableNamesOrEmptyWithLock()without initializing any table. Theper-table
SHOWprivilege filter is preserved (it is name-based and needs notable load), so no table name is leaked to a user who lacks
SHOWon it.Everything else keeps the original
getTables()loop unchanged:SHOW TABLES/SHOW FULL TABLES.SHOW FULL TABLES(verbose): it still needsgetMysqlType()and the storage-format columns, so it takes the original path and its
4-column output (
Table_type,Storage_format,Inverted_index_storage_format) is unchanged.SHOW VIEWS(needsgetEngine()to filter views) andSHOW STREAMS.Release note
None (performance optimization;
SHOW TABLESoutput for accessible tables isunchanged,
SHOW FULL TABLES/SHOW VIEWSare unaffected).Check List (For Author)
SHOW TABLESandSHOW TABLES LIKElist names via the fast path;SHOW FULL TABLESreturns the correct 4 columns (name,
Table_type,Storage_format,Inverted_index_storage_format) with no column mismatch.ShowTableCommandTest(2 passed); FE build (build.sh --fe)ExternalCatalog with a working
getTableNamesOrEmptyWithLock(), which isnot available in the FE unit-test harness; a portable regression
.outneeds the docker HMS test environment.
now listed by
SHOW TABLESwhere the old path silently omitted it on loadfailure, which is arguably more correct).