morningman commented on PR #66116:
URL: https://github.com/apache/doris/pull/66116#issuecomment-5158690509
## 1. [Must fix] Repair the merge fallout: restore imports and calls
consistent with current master
The branch currently references classes that do not exist, so the FE module
cannot compile. `ShowTabletsFromTableCommand.java` imports
`org.apache.doris.info.PartitionNamesInfo` and
`org.apache.doris.info.TableNameInfo`, but both classes now live in the
`fe-catalog` module under `org.apache.doris.catalog.info` (the
`org.apache.doris.info` package only contains `TableNameInfoUtils`,
`TableRefInfo`, and `TableValuedFunctionRefInfo`). Likewise,
`dbTableName.analyze(ctx)` passes a `ConnectContext`, while the current
signature is `analyze(NameSpaceContext)` — master calls
`dbTableName.analyze(ctx.getNameSpaceContext())`.
This happened because the branch was created before the fe-catalog
refactoring, and both merges of master (`7cdd6d84544`, `cce86ad7f73`) resolved
conflicts in this file by keeping the stale side, effectively reverting the
upstream refactor. CI has not exposed it yet because the compile pipelines have
not been triggered (they require a committer to comment `run buildall`).
Rebase onto the latest master (rather than merging master in again), keep
only the lines this fix genuinely needs to change, and restore master's imports
and the `analyze(ctx.getNameSpaceContext())` call. Before pushing, verify
locally with `mvn -pl fe-core -am compile` or trigger `run buildall`.
Everything else in this review assumes this is fixed first.
## 2. [Must fix] Actually wire up the `LIMIT 0` semantics
The parser now distinguishes "no LIMIT clause" (`-1`) from an explicit
`LIMIT 0` (`0`), and `RecordPickerUtils.getQualifiedRecords` handles
`Optional.of(0)` correctly (with a unit test). But the mapping in `doRun` still
only produces a size limit when `limit > 0`:
```java
Optional<Integer> sizeLimit = Optional.empty();
if (offset > 0 && limit > 0) {
sizeLimit = Optional.of((int) (offset + limit));
} else if (limit > 0) { // neither branch taken when limit == 0
sizeLimit = Optional.of((int) limit);
}
```
With `limit == 0`, `sizeLimit` stays empty and the utility falls back to
"take everything". As a result:
- `SHOW TABLETS ... LIMIT 0` still returns **all rows** (should return an
empty set);
- `SHOW TABLETS ... LIMIT 0 OFFSET 5` returns the **tail** of the sorted
result (should return an empty set).
In other words, the intent expressed by the parser change and by
`testZeroLimitReturnsNoRecords` never takes effect end to end — the gap sits
exactly in the one layer (the `sizeLimit` mapping) that has no test coverage. A
unified formula fixes all cases (`limit >= 0` means an explicit LIMIT was
given):
```java
Optional<Integer> sizeLimit = Optional.empty();
if (limit >= 0) {
long capped = Math.min(offset, Integer.MAX_VALUE) + Math.min(limit,
Integer.MAX_VALUE);
sizeLimit = Optional.of((int) Math.min(capped, Integer.MAX_VALUE));
}
```
This covers `LIMIT 0` (empty), `LIMIT 0 OFFSET m` (empty), and `LIMIT n
[OFFSET m]` (correct window), and also resolves the overflow issue in
suggestion 5. Please add a `doRun`-level test for `limit = 0` — that is
precisely the layer the current test matrix misses.
## 3. Clean up `RecordPickerUtils` and make its contract explicit
- Remove the commented-out dead code line `//Collections.sort(comparables,
comparator);`.
- The method has a hidden side effect: it **sorts the input list in place**,
then returns a copy of the sublist. Passing an immutable list would throw
`UnsupportedOperationException`. Either document clearly that the input gets
sorted, or copy first and sort the copy.
- Rename the parameter `k` to something meaningful such as `limit`. The
current javadoc ("Qualifies the record(s) that are available to be picked...")
does not convey the actual semantics — "sort, then truncate to the first N".
The early return on `comparables.isEmpty()` is redundant (sorting plus
`subList` on an empty list already yields an empty result) and can be dropped.
- Reconsider placement and naming: the utility has nothing Nereids-specific
— it operates on proc-dir-style `List<List<Comparable>>` rows.
`org.apache.doris.common.util` (next to `ListComparator` and `OrderByPair`) is
a more natural home, and a name like `sortAndLimit` is more direct than
`getQualifiedRecords`. Seven other command classes in the commands package
implement the same inline "sort + truncate" pattern with `ListComparator`; once
the utility lives in the right place, they can adopt it incrementally.
## 4. Preserve the early-stop optimization when there is no ORDER BY
(performance)
The old implementation stopped collecting tablet rows once `sizeLimit` rows
had been gathered. That early stop is what made `SHOW TABLETS ... LIMIT 10`
cheap on large tables. The new code unconditionally materializes every matching
row and fully sorts them (by the default `(tabletId, replicaId)` comparator)
while holding `olapTable.readLock()` — for tables with hundreds of thousands of
tablets times replicas, that is a real regression for a common ops command.
Full collection is a necessary cost for a user-specified ORDER BY (that is
the bug being fixed), but when `orderByPairs == null` the result is only
default-ordered and the early stop can be safely restored. Optionally, ORDER BY
with a small LIMIT could use a bounded priority queue (top-K heap, O(n log k))
instead of a full sort — nice to have, not required.
## 5. Guard the `(int)` casts against overflow
`Optional.of((int) (offset + limit))` overflows for values such as `LIMIT
3000000000` (both values come from `Long.parseLong`), producing a negative int
and ultimately an `IndexOutOfBoundsException` from `subList`, so the command
fails with an internal error. The old code had the same flaw, so this is not a
regression — but since this PR rewrites exactly these lines, clamping is
essentially free. Adopting the formula in suggestion 2 resolves this
automatically.
## 6. Add a regression test and correct the PR checklist
- Add `regression-test` cases covering `SHOW TABLETS ... ORDER BY TabletId
DESC LIMIT n`, `LIMIT n OFFSET m`, and `LIMIT 0`. Use a deterministic ordering
column such as `TabletId` — `LocalDataSize` is not stable in the regression
environment. No dedicated SHOW TABLETS ORDER BY suite exists today (only
incidental usages in e.g. segcompaction suites), and a regression case is also
the most convincing proof that the issue's scenario is fixed.
- The checklist claims "Regression test", but the diff contains only FE unit
tests — either add the regression case or check "Unit Test" instead.
- "Behavior changed: No" is inaccurate: `ORDER BY + LIMIT` results change
from wrong to correct, and `LIMIT n` without ORDER BY now returns the globally
smallest-tabletId rows instead of an arbitrary collection-order prefix. Mark it
"Yes" and describe it as a bug-fix behavior correction.
- "Does this need documentation? Yes" is checked but no documentation PR is
linked — please add one.
- Finally, please verify the original issue #65871 statement (`ORDER BY`
**without** LIMIT) on a real cluster: by code reading, master (and the
4.0.2-rc02 tag) already sorted in that path, so it is worth confirming whether
this PR truly resolves the reported symptom or whether that symptom has a
different, version-specific cause.
--
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]
