This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 788e488237d [fix](show) apply SHOW TABLETS ordering and LIMIT
independently (#66713)
788e488237d is described below
commit 788e488237d3e07f6f63a7d0d7e27e5902da6832
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Fri Aug 14 12:00:53 2026 +0800
[fix](show) apply SHOW TABLETS ordering and LIMIT independently (#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).
---
.../doris/nereids/parser/LogicalPlanBuilder.java | 9 +-
.../commands/ShowTabletsFromTableCommand.java | 101 +++++++++++++--------
.../commands/ShowTabletsFromTableCommandTest.java | 73 +++++++++++++++
.../suites/show_p0/test_show_tablet.groovy | 32 +++++--
4 files changed, 164 insertions(+), 51 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
index 8663577809f..05336a7771e 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
@@ -7842,12 +7842,13 @@ public class LogicalPlanBuilder extends
DorisParserBaseVisitor<Object> {
if (ctx.sortClause() != null) {
orderKeys = visit(ctx.sortClause().sortItem(), OrderKey.class);
}
- long limit = 0;
+ // -1 means the statement carries no LIMIT clause at all, which is not
the same as an
+ // explicit LIMIT 0: the former returns every row, the latter returns
none.
+ long limit = -1;
long offset = 0;
if (ctx.limitClause() != null) {
- limit = ctx.limitClause().limit != null
- ? Long.parseLong(ctx.limitClause().limit.getText())
- : 0;
+ // every alternative of the limitClause rule binds `limit`, so it
is never null here
+ limit = Long.parseLong(ctx.limitClause().limit.getText());
if (limit < 0) {
throw new ParseException("Limit requires non-negative number",
ctx.limitClause());
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommand.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommand.java
index 6c6ffca87ab..33535f80300 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommand.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommand.java
@@ -54,6 +54,7 @@ import org.apache.doris.qe.ShowResultSet;
import org.apache.doris.qe.ShowResultSetMetaData;
import org.apache.doris.qe.StmtExecutor;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import java.util.ArrayList;
@@ -70,7 +71,8 @@ public class ShowTabletsFromTableCommand extends ShowCommand {
private PartitionNamesInfo partitionNames;
private Expression whereClause;
private List<OrderKey> orderKeys;
- private long limit = 0;
+ // -1 means no LIMIT clause was given; 0 means an explicit LIMIT 0
+ private long limit = -1;
private long offset = 0;
private long version;
@@ -193,6 +195,29 @@ public class ShowTabletsFromTableCommand extends
ShowCommand {
throw new AnalysisException("Title name[" + columnName + "] does not
exist");
}
+ /**
+ * Maps the parsed LIMIT/OFFSET pair onto how many rows have to be kept
before the OFFSET is
+ * applied, that is the LIMIT rows plus the OFFSET rows that are skipped
afterwards.
+ *
+ * <p>A negative {@code limit} means the statement carried no LIMIT clause
at all
+ * (see {@link
org.apache.doris.nereids.parser.LogicalPlanBuilder#visitShowTabletsFromTable}),
+ * so the result is unbounded; {@code limit == 0} is an explicit LIMIT 0
and bounds the result
+ * to nothing. Each operand is clamped to {@link Integer#MAX_VALUE} before
they are added,
+ * because a huge LIMIT/OFFSET pair would otherwise overflow long and end
up as a negative
+ * size.
+ *
+ * @return the number of rows to keep, or {@link Optional#empty()} for "no
bound at all"
+ */
+ @VisibleForTesting
+ static Optional<Integer> computeSizeLimit(long limit, long offset) {
+ if (limit < 0) {
+ return Optional.empty();
+ }
+ long capped = Math.min(limit, Integer.MAX_VALUE)
+ + Math.min(Math.max(offset, 0), Integer.MAX_VALUE);
+ return Optional.of((int) Math.min(capped, Integer.MAX_VALUE));
+ }
+
@Override
public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws Exception {
validate(ctx);
@@ -200,18 +225,12 @@ public class ShowTabletsFromTableCommand extends
ShowCommand {
Env env = Env.getCurrentEnv();
Database db =
env.getInternalCatalog().getDbOrAnalysisException(dbTableName.getDb());
OlapTable olapTable =
db.getOlapTableOrAnalysisException(dbTableName.getTbl());
+
+ Optional<Integer> sizeLimit = computeSizeLimit(limit, offset);
+
+ List<List<Comparable>> tabletInfos = new ArrayList<>();
olapTable.readLock();
try {
- // The parser passes limit = 0 when the statement carries no LIMIT
clause
- // (see LogicalPlanBuilder#visitShowTabletsFromTable), so only a
positive limit
- // bounds the result. sizeLimit is how many sorted rows have to be
kept: the LIMIT
- // rows plus the OFFSET rows that are skipped afterwards.
- Optional<Integer> sizeLimit = Optional.empty();
- if (limit > 0) {
- long capped = limit + Math.max(offset, 0);
- sizeLimit = Optional.of((int) Math.min(capped,
Integer.MAX_VALUE));
- }
-
Collection<Partition> partitions = new ArrayList<Partition>();
if (partitionNames != null) {
List<String> paNames = partitionNames.getPartitionNames();
@@ -230,10 +249,10 @@ public class ShowTabletsFromTableCommand extends
ShowCommand {
// With an explicit ORDER BY every tablet has to be collected
before the result can be
// truncated, otherwise the sort only sees an arbitrary prefix of
the scan and returns
// the wrong rows -- the bug reported in #65871. Without ORDER BY
the scan still stops
- // as soon as enough rows are gathered, as it did before: LIMIT
then returns a prefix
- // of the scan, ordered by (tabletId, replicaId) among itself.
- boolean stop = false;
- List<List<Comparable>> tabletInfos = new ArrayList<>();
+ // as soon as enough rows are gathered, as it did before.
+ // An explicit LIMIT 0 cannot return any row, so nothing has to be
fetched at all,
+ // whether or not an OFFSET was given.
+ boolean stop = limit == 0;
for (Partition partition : partitions) {
if (stop) {
break;
@@ -248,31 +267,39 @@ public class ShowTabletsFromTableCommand extends
ShowCommand {
}
}
}
+ } finally {
+ olapTable.readUnlock();
+ }
- ListComparator<List<Comparable>> comparator;
- if (orderByPairs != null) {
- // order by the keys given by the user
- OrderByPair[] orderByPairArr = new
OrderByPair[orderByPairs.size()];
- comparator = new
ListComparator<>(orderByPairs.toArray(orderByPairArr));
- } else {
- // order by tabletId, replicaId
- comparator = new ListComparator<>(0, 1);
- }
- List<List<Comparable>> orderedTabletInfos =
SortAndLimit.sortAndLimit(tabletInfos, comparator, sizeLimit);
+ // Every row holds values copied out of the catalog, so sorting and
formatting them no
+ // longer needs the table lock.
+ List<List<Comparable>> resultInfos;
+ if (orderByPairs != null) {
+ // the ORDER BY given by the user applies to the whole tablet set
of the table
+ OrderByPair[] orderByPairArr = new
OrderByPair[orderByPairs.size()];
+ resultInfos = SortAndLimit.sortAndLimit(tabletInfos,
+ new
ListComparator<>(orderByPairs.toArray(orderByPairArr)), sizeLimit);
+ } else if (sizeLimit.isPresent()) {
+ // No ORDER BY and a LIMIT: the scan stopped as soon as enough
rows were gathered, so
+ // what was collected is an arbitrary subset of the table. Sorting
it here would only
+ // make that subset look like the globally smallest rows, so the
rows are left in scan
+ // order and only their number is bounded.
+ resultInfos = tabletInfos.subList(0, Math.min(sizeLimit.get(),
tabletInfos.size()));
+ } else {
+ // No ORDER BY and no LIMIT: every row is collected anyway, so
keep the
+ // (tabletId, replicaId) ordering this command has always returned
in that case.
+ resultInfos = SortAndLimit.sortAndLimit(tabletInfos, new
ListComparator<>(0, 1), Optional.empty());
+ }
- // If offset is beyond the end of the result, subList yields an
empty list and no row
- // is returned.
- int resultOffset = (int) Math.min(offset,
orderedTabletInfos.size());
- for (List<Comparable> tabletInfo
- : orderedTabletInfos.subList(resultOffset,
orderedTabletInfos.size())) {
- List<String> oneTablet = new
ArrayList<String>(tabletInfo.size());
- for (Comparable column : tabletInfo) {
- oneTablet.add(column.toString());
- }
- rows.add(oneTablet);
+ // If offset is beyond the end of the result, subList yields an empty
list and no row
+ // is returned.
+ int resultOffset = (int) Math.min(offset, resultInfos.size());
+ for (List<Comparable> tabletInfo : resultInfos.subList(resultOffset,
resultInfos.size())) {
+ List<String> oneTablet = new ArrayList<String>(tabletInfo.size());
+ for (Comparable column : tabletInfo) {
+ oneTablet.add(column.toString());
}
- } finally {
- olapTable.readUnlock();
+ rows.add(oneTablet);
}
return new ShowResultSet(getMetaData(), rows);
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommandTest.java
b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommandTest.java
new file mode 100644
index 00000000000..d471e182afc
--- /dev/null
+++
b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommandTest.java
@@ -0,0 +1,73 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees.plans.commands;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.Optional;
+
+/**
+ * Covers the mapping from the parsed LIMIT/OFFSET pair onto the number of
rows SHOW TABLETS has
+ * to keep. The end-to-end row selection is covered by the
show_p0/test_show_tablet regression
+ * suite; this test pins down the arithmetic, including the two cases that are
easy to get wrong:
+ * "no LIMIT clause" vs "LIMIT 0", and the overflow of LIMIT + OFFSET.
+ */
+public class ShowTabletsFromTableCommandTest {
+
+ @Test
+ public void testNoLimitClauseIsUnbounded() {
+ // the parser passes -1 when the statement carries no LIMIT clause at
all
+ Assertions.assertEquals(Optional.empty(),
ShowTabletsFromTableCommand.computeSizeLimit(-1, 0));
+ }
+
+ @Test
+ public void testExplicitZeroLimitKeepsNoRow() {
+ Assertions.assertEquals(Optional.of(0),
ShowTabletsFromTableCommand.computeSizeLimit(0, 0));
+ }
+
+ @Test
+ public void testZeroLimitWithOffsetStillKeepsNoRow() {
+ // LIMIT 5, 0 keeps 5 rows here, but all of them are dropped by the
OFFSET afterwards
+ Assertions.assertEquals(Optional.of(5),
ShowTabletsFromTableCommand.computeSizeLimit(0, 5));
+ }
+
+ @Test
+ public void testLimitWithoutOffset() {
+ Assertions.assertEquals(Optional.of(10),
ShowTabletsFromTableCommand.computeSizeLimit(10, 0));
+ }
+
+ @Test
+ public void testLimitAndOffsetAreAddedUp() {
+ Assertions.assertEquals(Optional.of(13),
ShowTabletsFromTableCommand.computeSizeLimit(3, 10));
+ }
+
+ @Test
+ public void testLargeLimitIsClampedToIntRange() {
+ Assertions.assertEquals(Optional.of(Integer.MAX_VALUE),
+ ShowTabletsFromTableCommand.computeSizeLimit(3000000000L, 0));
+ }
+
+ @Test
+ public void testHugeLimitAndOffsetDoNotOverflow() {
+ // both operands come from Long.parseLong, so adding them before
clamping would wrap
+ // around into a negative size and later blow up in List#subList
+ Assertions.assertEquals(Optional.of(Integer.MAX_VALUE),
+ ShowTabletsFromTableCommand.computeSizeLimit(Long.MAX_VALUE,
Long.MAX_VALUE));
+ }
+}
diff --git a/regression-test/suites/show_p0/test_show_tablet.groovy
b/regression-test/suites/show_p0/test_show_tablet.groovy
index 51e5e4fae46..b691fb9a982 100644
--- a/regression-test/suites/show_p0/test_show_tablet.groovy
+++ b/regression-test/suites/show_p0/test_show_tablet.groovy
@@ -89,7 +89,9 @@ suite("test_show_tablet") {
def descIds = new ArrayList(ascIds)
Collections.reverse(descIds)
- // without ORDER BY the rows come back ordered by (TabletId, ReplicaId)
+ // Without ORDER BY and without LIMIT every tablet is collected anyway, so
the whole result
+ // is returned ordered by (TabletId, ReplicaId). Note this holds only for
the unbounded
+ // result -- see the LIMIT cases below, where no ordering is promised.
assertEquals(ascIds, allIds)
// ORDER BY without LIMIT returns every tablet
@@ -105,27 +107,37 @@ suite("test_show_tablet") {
res = sql """SHOW TABLETS FROM show_tablets_multi_part_t ORDER BY TabletId
DESC LIMIT 2, 3"""
assertEquals(descIds.subList(2, 5), res.collect { it[0] as long })
- // Without ORDER BY the scan stops as soon as enough rows are gathered, so
LIMIT returns a
- // prefix of the scan rather than the globally smallest tablet ids. Only
the row count and
- // the ordering inside the returned prefix are guaranteed.
- def assertPrefixOfTable = { rows, expectedSize ->
+ // Without ORDER BY the scan stops as soon as enough rows are gathered, so
what comes back is
+ // an arbitrary subset of the table -- which partition is walked first is
not defined. The
+ // rows are deliberately not sorted either, because sorting an arbitrary
subset would make it
+ // look like the globally smallest tablet ids. Only the row count and the
fact that the rows
+ // belong to this table are guaranteed.
+ def assertAnySubsetOfTable = { rows, expectedSize ->
assertEquals(expectedSize, rows.size())
def ids = rows.collect { it[0] as long }
- def sortedIds = new ArrayList(ids)
- Collections.sort(sortedIds)
- assertEquals(sortedIds, ids)
assertTrue(allIds.containsAll(ids))
}
res = sql """SHOW TABLETS FROM show_tablets_multi_part_t LIMIT 3"""
- assertPrefixOfTable(res, 3)
+ assertAnySubsetOfTable(res, 3)
res = sql """SHOW TABLETS FROM show_tablets_multi_part_t LIMIT 2, 3"""
- assertPrefixOfTable(res, 3)
+ assertAnySubsetOfTable(res, 3)
// an offset past the end of the result yields no row
res = sql """SHOW TABLETS FROM show_tablets_multi_part_t LIMIT
${allTablets.size()}, 3"""
assertTrue(res.isEmpty())
+ // LIMIT 0 asks for no row and must not fall back to "no limit at all",
with or without
+ // an OFFSET and with or without an ORDER BY
+ res = sql """SHOW TABLETS FROM show_tablets_multi_part_t LIMIT 0"""
+ assertTrue(res.isEmpty())
+
+ res = sql """SHOW TABLETS FROM show_tablets_multi_part_t LIMIT 2, 0"""
+ assertTrue(res.isEmpty())
+
+ res = sql """SHOW TABLETS FROM show_tablets_multi_part_t ORDER BY TabletId
DESC LIMIT 0"""
+ assertTrue(res.isEmpty())
+
sql """drop table if exists show_tablets_multi_part_t;"""
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
