morrySnow commented on code in PR #66713:
URL: https://github.com/apache/doris/pull/66713#discussion_r3773480204
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletsFromTableCommand.java:
##########
@@ -248,31 +267,39 @@ public ShowResultSet doRun(ConnectContext ctx,
StmtExecutor executor) throws Exc
}
}
}
+ } 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()));
Review Comment:
Low severity (consistency, not correctness). This branch is taken for any
`LIMIT n` without `ORDER BY`, including when `n >= ` the total tablet count. In
that case the scan loop never hits `tabletInfos.size() >= sizeLimit` (line 264)
and does not stop early, so `tabletInfos` is actually the complete tablet set —
not an arbitrary subset — yet it is returned in raw scan order instead of the
`(TabletId, ReplicaId)` order that `SHOW TABLETS FROM t` (no LIMIT) returns via
the `else` branch. A client/script that appends a generous LIMIT to 'get
everything' will observe a different ordering than the unbounded form. Since
the rows are already complete here, they could be sorted consistently. Consider
keying this branch on whether the scan was actually truncated (e.g., whether
`stop` was set by the size check) rather than on `sizeLimit.isPresent()`.
##########
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
Review Comment:
Nit: `computeSizeLimit(0, 5)` returns `Optional.of(5)`, but `doRun`
short-circuits with `stop = limit == 0` and never fetches these rows. So "keeps
5 rows here, but all of them are dropped by the OFFSET afterwards" doesn't
describe what the command actually does — for `LIMIT 5, 0` the scan is skipped
entirely and zero rows are kept. The returned positive value only selects the
`else if` branch and is otherwise unused in the `limit == 0` case. Consider
rewording the comment (or having `computeSizeLimit` return `Optional.of(0)` for
`limit == 0`), since the test name says "keeps no row" while the assertion pins
a positive size.
--
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]
