Skip to content

[test](catalog) Add unit tests for SHOW TABLES name listing on external catalogs - #1

Merged
Baymine merged 3 commits into
Baymine:improvement/show-tables-external-perffrom
u70b3:test-show-tables-external-ut
Aug 5, 2026
Merged

[test](catalog) Add unit tests for SHOW TABLES name listing on external catalogs#1
Baymine merged 3 commits into
Baymine:improvement/show-tables-external-perffrom
u70b3:test-show-tables-external-ut

Conversation

@u70b3

@u70b3 u70b3 commented Aug 4, 2026

Copy link
Copy Markdown

Hi @Baymine — as mentioned in apache#66080, here are unit tests for the SHOW TABLES fast path, adapted to your implementation (getTableNamesWithLock() and the InternalCatalog check).

7 cases:

  • plain SHOW TABLES on an external catalog lists names only — verifies getTables() is never called
  • LIKE pattern filter on the fast path
  • SHOW privilege filter on the fast path
  • name-listing failure (case-insensitive conflict) propagates instead of returning an empty result
  • SHOW FULL TABLES / SHOW VIEWS / internal catalog keep the getTables() path (temporary tables still hidden)

Verified: mvn -pl fe-core -am test -Dtest=ShowTableCommandExternalTest → 7/7 pass; fe-core checkstyle clean.

If you merge this into your branch, the tests become part of apache#66080. Happy to adjust anything.

…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)
@u70b3

u70b3 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Note on the two red checks — both are fork-side CI infrastructure issues, unrelated to this change (which adds a single Java test file):

  • ShellCheck: GitHub refuses to check out fork-PR code in a pull_request_target workflow (cross-fork anti-pwn-request protection). Expected for any fork-to-fork PR.
  • Clang Tidy Preparation: the fork's workflow files pin the deprecated actions/upload-artifact: v3, which GitHub now hard-fails. These workflows no longer exist in current apache/doris master — syncing the fork with upstream would clear them, but it's not required.

Neither check processes Java files, and the real CI gate runs on apache#66080 after merge. Safe to merge despite the red X's here.

@Baymine

Baymine commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Thanks for the tests, @u70b3 — the failure-propagation case in particular is a good catch.

One thing before we merge: I'd like to keep this aligned with the Google Java Style Guide on test-class naming:

A test class has a name that ends with Test, for example, HashIntegrationTest. If it covers a single class, its name is the name of that class plus Test, for example HashImplTest.

Both this new ShowTableCommandExternalTest and the existing ShowTableCommandTest cover the same single class, ShowTableCommand — so per the guide the "external" cases should live in ShowTableCommandTest rather than in a separate class. Splitting one class's tests across
two files also makes coverage harder to see at a glance.

ShowTableCommandTest already added fast-path coverage in cad2f74, so most of the 7 cases here overlap with what's there:

  • listsNamesOnly / privilege filter → already covered
  • SHOW FULL TABLES / SHOW VIEWS / internal-catalog still-loads → already covered

The two that add genuinely new coverage are:

  • LIKE-pattern filter on the fast path
  • name-listing failure propagates (guards the getTableNamesWithLock() contract from 6f15009, which currently has no FE unit test)

Could you fold just those two cases into ShowTableCommandTest (reusing its existing ShowTableCommandMocks helper) and drop the new class? That keeps one test file per class and avoids the duplicate cases.

u70b3 added 2 commits August 5, 2026 16:39
…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.
@Baymine
Baymine merged commit 52dc351 into Baymine:improvement/show-tables-external-perf Aug 5, 2026
4 of 7 checks passed
@u70b3

u70b3 commented Aug 5, 2026

Copy link
Copy Markdown
Author

@Baymine Done — folded the LIKE-pattern and name-conflict propagation cases into ShowTableCommandTest (reusing ShowTableCommandMocks), dropped ShowTableCommandExternalTest and the 5 overlapping cases. Also merged your cad2f74d into the branch to pick up the mocks helper. Verified: mvn -pl fe-core -am test -Dtest=ShowTableCommandTest → 8/8 pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants