morningman commented on PR #63860:
URL: https://github.com/apache/doris/pull/63860#issuecomment-4610942691
## 1. [Medium] Add a test that actually locks in the fix
This is the most valuable improvement. As it stands, **no test would fail if
bug #1 (the missing `return`) were reintroduced.**
The regression helper `checkNereidsExecute` (defined at `Suite.groovy:1003`)
is simply `sql(sqlString)` — it asserts only that the statement executes
without throwing, and never inspects the result. The new regression cases use
fake IPs (`192.168.0.1~3:9050`) plus `127.0.0.1:9050`, none of which match a
real backend in the test cluster. The consequence: with the bug present the
statement returns all backends, and after the fix it returns an empty set — but
`checkNereidsExecute` ignores the output in both cases, so the test passes
either way. The FE incremental unit-test coverage report confirms this: `0.00%
(0/16)`.
The output of `SHOW TRASH ON` depends on the cluster and backend IPs, so a
`qt` result assertion isn't practical. The right place to pin the semantics is
a lightweight unit test on the parser output — which also exercises the newly
added `getBackendsQuery()` getter:
```java
// 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
`ON` clause takes effect." A small assertion on the parsed command nails down
the core semantics of this fix at very low cost.
## 2. [Low] Surface backends that don't match
An unknown or mistyped backend string (e.g. `SHOW TRASH ON
("nonexist:9050")`) is silently dropped and yields an empty result set, with no
error or warning. The user can't tell "this backend has no trash" from "I typed
the host:port wrong." Consider collecting the unmatched query strings and
emitting a `LOG.warn`, or otherwise signaling them.
**Note**: this behavior is inherited from `AdminCleanTrashCommand`. If you
decide to improve it, change **both** places together — fixing only one would
reintroduce the very inconsistency this PR set out to remove. Low priority, and
pre-existing.
## 3. [Low] Make sure the user-facing syntax change is documented
Requiring parentheses is a hard, user-facing syntax break: the old `SHOW
TRASH ON "be:9050"` (no parens) no longer parses. The PR checks "No need
documentation" in this repo (user docs live in the separate `doris-website`
repo), which is reasonable for the code repo — but the `SHOW TRASH` SQL
reference should be updated to the new syntax, and the release note should
state clearly that the old non-parenthesized form is no longer supported.
--
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]
