[fix](fe) Fix SHOW TRASH ON clause to support multiple backends#63860
Open
heguanhui wants to merge 1 commit into
Open
[fix](fe) Fix SHOW TRASH ON clause to support multiple backends#63860heguanhui wants to merge 1 commit into
heguanhui wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31218 ms |
Contributor
TPC-DS: Total hot run time: 172298 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31141 ms |
heguanhui
force-pushed
the
fix/show-trash-on-multi-be
branch
from
7db0866 to
d7b4c0e
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-DS: Total hot run time: 170838 ms |
Contributor
TPC-H: Total hot run time: 31100 ms |
Contributor
TPC-DS: Total hot run time: 171892 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary for PR #63860 at d7b4c0e.
No blocking issues found.
Critical checkpoint conclusions:
- Goal and proof: The PR fixes the Nereids
SHOW TRASH ONpath so the parsed ON clause is returned and executed, and extends the syntax to the same parenthesized backend-list shape used byADMIN CLEAN TRASH ON. The regression test covers parsing/execution ofSHOW TRASH, single-backend ON, and multi-backend ON forms. - Scope: The change is small and focused across grammar, plan construction, command backend filtering, and the existing regression test.
- Concurrency: No new shared concurrency or locking behavior is introduced. Backend metadata is read through the existing system-info snapshot path, and the command performs the same BE RPC flow already used by
TrashProcDir. - Lifecycle/static initialization: No new static/global lifecycle dependency or cross-translation-unit/static initialization issue is introduced.
- Configuration: No configuration item is added.
- Compatibility/storage/protocol: No storage format, function symbol, or FE-BE protocol field change is introduced. The SQL syntax intentionally changes from
SHOW TRASH ON "be:port"toSHOW TRASH ON ("be:port", ...), matching the PR release note andADMIN CLEAN TRASH ONsyntax. - Parallel paths: The backend-list filtering now mirrors the existing
AdminCleanTrashCommandpath; no otherSHOW TRASHimplementation path was found in FE Java code. - Conditional checks: The privilege check and backend-selection conditions follow existing adjacent command behavior. Unknown backend strings still produce an empty backend list, matching the existing clean-trash command semantics.
- Test coverage: Regression coverage exists for the new Nereids syntax. I did not run the regression suite in this review environment.
- Test results: No
.outfile is involved because the test usescheckNereidsExecutefor cluster-dependent output. - Observability: No new observability requirement was identified for this small command/parser fix; existing warning logs in
TrashProcDirremain the runtime signal for BE RPC failures. - Transactions/persistence/data writes:
SHOW TRASHis read-only and does not touch transaction or persistence state. The PR does not modifyADMIN CLEAN TRASHwrite/task submission behavior. - FE/BE variables: No new FE-to-BE variable or thrift field is added.
- Performance: Backend lookup changes are O(number of backends + requested backends), which is appropriate for this administrative command.
User focus points: No additional user-provided review focus was specified.
Contributor
1. [Medium] Add a test that actually locks in the fixThis is the most valuable improvement. As it stands, no test would fail if bug #1 (the missing The regression helper The output of // Parse SHOW TRASH ON ("be1:9050", "be2:9050") and assert it yields a
// ShowTrashCommand whose backendsQuery == ["be1:9050", "be2:9050"].
// If anyone deletes the `return` in visitShowTrash again, this test should fail immediately.
LogicalPlan plan = parser.parseSingle("SHOW TRASH ON (\"be1:9050\", \"be2:9050\")");
Assertions.assertTrue(plan instanceof ShowTrashCommand);
Assertions.assertEquals(
Lists.newArrayList("be1:9050", "be2:9050"),
((ShowTrashCommand) plan).getBackendsQuery());Why it matters: the current regression cases prove "it runs," not "the 2. [Low] Surface backends that don't matchAn unknown or mistyped backend string (e.g. Note: this behavior is inherited from 3. [Low] Make sure the user-facing syntax change is documentedRequiring parentheses is a hard, user-facing syntax break: the old |
heguanhui
force-pushed
the
fix/show-trash-on-multi-be
branch
from
d7b4c0e to
3f5ecce
Compare
heguanhui
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
Author
|
run buildall |
heguanhui
force-pushed
the
fix/show-trash-on-multi-be
branch
from
3f5ecce to
d63cab3
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 30122 ms |
Contributor
TPC-DS: Total hot run time: 180572 ms |
Contributor
ClickBench: Total hot run time: 24.89 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary: SHOW TRASH ON clause has two bugs:
LogicalPlanBuilder.visitShowTrash(), causingSHOW TRASH ON "be:9050"to always return trash info for all backends instead of the specified one.SHOW TRASH ONonly supports a single backend string literal, whileADMIN CLEAN TRASH ONsupports multiple backends with parenthesized comma-separated list. The syntax is inconsistent between the two related statements.Release note
Fix SHOW TRASH ON clause to support multiple backends with the same syntax as ADMIN CLEAN TRASH ON, e.g.
SHOW TRASH ON ("be1:9050", "be2:9050"). Previously the ON clause was silently ignored due to a missing return.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)