morningman opened a new pull request, #66713:
URL: https://github.com/apache/doris/pull/66713
### What problem does this PR solve?
Issue Number: related to #65871
Related PR: follow-up to #66116
Problem Summary:
#66116 fixed `SHOW TABLETS ... ORDER BY ... LIMIT n` so that the sort sees
the whole tablet set instead of an arbitrary prefix of the scan. That part is
correct and is kept as is. But the fix routed both the ORDER BY branch and the
branch without ORDER BY through one `SortAndLimit` call, and that helper needs
a comparator even when no ordering was asked for. Three problems follow from
that, plus one that was never covered by a test.
**1. Without ORDER BY, an arbitrary subset was sorted and presented as if it
were a global top-N.**
When no ORDER BY is given, the scan stops as soon as enough rows are
gathered, so the collected rows are an arbitrary subset of the table. Which
partition is walked first comes from `ConcurrentHashMap` iteration order
(`OlapTable#getPartitions` returns `idToPartition.values()`), so it is neither
id order nor creation order, and it shifts when partitions are added or
dropped. Sorting that subset by `(TabletId, ReplicaId)` produced a clean
ascending column that looks like the globally smallest tablet ids but is not.
The trap is that it *is* correct on a table with a single partition and no
rollup: one index is materialized in full before the size check, so the
collected set is the whole table. The discrepancy only shows up on partitioned
tables, which is exactly where it will not be noticed during testing.
```sql
-- p1 = 10001..10003, p2 = 10004..10006, p3 = 10007..10009
SHOW TABLETS FROM t LIMIT 3;
-- returns e.g. 10007, 10008, 10009 -- neatly ascending, but not the 3
smallest,
-- and a later ADD PARTITION can change which three come back
```
This PR returns those rows in scan order and bounds only their number.
Sorting an arbitrary subset cannot be made meaningful, so it is better not to
imply an order that is not there. Without a LIMIT every row is collected
anyway, so that case keeps the `(TabletId, ReplicaId)` ordering the command has
always returned.
**2. `LIMIT 0` returned the whole table.**
`LogicalPlanBuilder#visitShowTabletsFromTable` used `0` both for "the
statement has no LIMIT clause" and for an explicit `LIMIT 0`, so the command
could not tell them apart and fell back to "no limit at all".
```sql
SHOW TABLETS FROM t LIMIT 0; -- returned every tablet, MySQL semantics
say no row
SHOW TABLETS FROM t LIMIT 5, 0; -- returned everything past the offset
```
The parser now passes `-1` for a missing LIMIT clause, so `limit == 0`
bounds the result to nothing and the scan is skipped entirely.
**3. `limit + offset` overflowed.**
Both operands come from `Long.parseLong`, and the sum wrapped into a
negative size that later reached `List#subList`:
```sql
SHOW TABLETS FROM t LIMIT 9223372036854775807, 9223372036854775807;
-- IndexOutOfBoundsException
```
Each operand is now clamped to `Integer.MAX_VALUE` before they are added,
which restores a guard that #66116 had removed. This is the case @morrySnow
raised on #66116 with `Utils#addOverflows`.
**4. The layer where these bugs keep landing had no test.**
Every `sizeLimit` bug so far has been in the mapping from LIMIT/OFFSET onto
the number of rows to keep. `SortAndLimitTest` only covers the utility, and the
command test only covers `validate()`, so that layer sat between two test
suites with none of its own. It is extracted into `computeSizeLimit()` and
covered directly.
**Additionally**: sorting and formatting move out of `olapTable.readLock()`.
`TabletsProcDir#fetchComparableResult` builds rows out of copied longs and
strings and keeps no reference to catalog objects, so the lock is only needed
for the scan. This matters because an explicit ORDER BY now has to collect the
whole tablet set: on a table with hundreds of thousands of tablets times
replicas, `SHOW TABLETS ... ORDER BY LocalDataSize DESC LIMIT 10` materializes
every row, and holding the table read lock across the sort and the string
conversion blocks schema change and partition DDL on that table for the whole
time.
### Resulting semantics
| statement | result |
|---|---|
| `SHOW TABLETS FROM t` | every row, ordered by (TabletId, ReplicaId) |
| `... ORDER BY k` | every row, ordered by k |
| `... ORDER BY k LIMIT n` | global top-n by k |
| `... ORDER BY k LIMIT m, n` | global rank m..m+n-1 by k |
| `... LIMIT n` | n arbitrary rows, no ordering promised |
| `... LIMIT m, n` | n arbitrary rows, no ordering promised |
| `... LIMIT 0` / `... LIMIT m, 0` | no row |
### Release note
Fix `SHOW TABLETS ... LIMIT 0`, which returned the whole table instead of an
empty result. Without an explicit `ORDER BY`, `SHOW TABLETS ... LIMIT n` no
longer sorts its result: the rows it returns are an arbitrary subset of the
table, and sorting them suggested a global order that was never there. The
unbounded `SHOW TABLETS FROM tbl` keeps returning every row ordered by
(TabletId, ReplicaId).
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [ ] No.
- [x] Yes. Three user visible changes, all listed in the release note
and in the table above: `LIMIT 0` and `LIMIT m, 0` now return no row instead of
the whole table; `LIMIT n` without `ORDER BY` no longer sorts its result;
`LIMIT` with a huge `OFFSET` no longer fails with an internal error. `SHOW
TABLETS` without a LIMIT is unchanged, and `ORDER BY` keeps the behavior #66116
established.
- Does this need documentation?
- [ ] No.
- [x] Yes. The docs do not currently describe an ordering guarantee for
`SHOW TABLETS` without `ORDER BY`, so nothing there becomes wrong, but the
`LIMIT 0` correction is worth a note. Will open a doris-website PR if reviewers
agree on the semantics above.
--
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]
