lets-order-some-fries opened a new pull request, #67008:
URL: https://github.com/apache/doris/pull/67008
### What problem does this PR solve?
Problem Summary: LIMIT and OFFSET reach the SHOW paging code as unbounded
64-bit
values. `limitClause` accepts a bare digit sequence, `LogicalPlanBuilder`
parses
it with `Long.parseLong` and checks only that it is non-negative, and
`LimitElement` stores both as `long`. Every paging site then narrowed them to
`int` before clamping, so a value above `Integer.MAX_VALUE` wrapped.
Two failure modes on a 5-row result:
- `LIMIT 4294967296` -> `(int) (0 + 4294967296L)` is 0, the `endIndex > size`
clamp does not fire, and `subList(0, 0)` returns an empty result. The user
gets
zero rows back with no error.
- `LIMIT 3000000000` -> `(int) 3000000000L` is -1294967296, which is not
greater
than size so it is not clamped; the `beginIndex > endIndex` branch then
sets
`beginIndex` to the same negative value and `subList` throws
`IndexOutOfBoundsException`. `OFFSET 3000000000` reaches the same throw.
The same block was duplicated at eight sites across four proc dirs, backing
`SHOW PARTITIONS`, `SHOW ALTER TABLE COLUMN`, `SHOW ALTER TABLE ROLLUP` and
`SHOW BUILD INDEX`. `RollupProcDir` additionally lacked the `beginIndex >
endIndex`
guard that its three siblings have, so it could call `subList` with
`beginIndex > endIndex` and throw `IllegalArgumentException`.
`ShowCommand.applyLimit`, which pages `SHOW LOAD`, `SHOW RESOURCES` and
`SHOW PARTITIONS`, has the same defect in a different shape: `(limit +
offsetValue)`
is computed in long but can overflow to a negative value, which then passes
the
`< showResult.size()` test and is narrowed for `subList`.
### What is changed and how does it work?
Added `LimitElement.applyTo(List)`, which owns both values already, and
computes
the window in long, saturating at `rows.size()` before narrowing:
- the offset is clamped into `[0, size]` first, so an out-of-range offset
yields
an empty window instead of a negative index;
- `begin + limit` is checked for a negative result, which is the signature
of the
long addition overflowing;
- with no limit set the window runs to the end of the list.
The eight duplicated blocks now delegate to it, which also gives
`RollupProcDir`
the guard it was missing. `ShowCommand.applyLimit` delegates too, keeping its
existing `limit == -1` contract (no limit means return the list unchanged) so
behaviour is identical apart from the overflow.
Net effect is 36 fewer lines and one tested implementation instead of eight
copies.
### Release note
Fix `SHOW PARTITIONS`, `SHOW ALTER TABLE`, `SHOW BUILD INDEX`, `SHOW LOAD`
and
`SHOW RESOURCES` returning an empty result or failing with
`IndexOutOfBoundsException` when LIMIT or OFFSET exceeds the 32-bit range.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] 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 <!-- Add your reason? -->
Added `LimitElementTest` (7 cases) covering both failure modes, an offset
past the end, `offset + limit` overflowing long, and the no-limit path.
```
mvn -f fe/pom.xml -pl :fe-core -am test \
-Dtest=LimitElementTest,PartitionsProcDirTest,AlterProcDirFilterExpressionTest,ShowPartitionsCommandTest
-> Tests run: 13, Failures: 0, Errors: 0, Skipped: 0
cd fe && mvn clean checkstyle:check
-> BUILD SUCCESS
```
I confirmed the new tests fail against the original arithmetic, so they
genuinely cover the defect rather than passing vacuously - 6 of 7 fail, 3 as
silently empty results and 3 as the predicted crash:
```
testLimitAboveIntMaxReturnsAllRows expected: <[0, 1, 2, 3, 4]> but was:
<[]>
testOffsetPlusLimitOverflowIsClamped expected: <[1, 2, 3, 4]> but was:
<[]>
testLimitTruncatingToNegativeIntReturnsAllRows
IndexOutOfBoundsException: fromIndex
= -1294967296
```
No end-to-end regression test was added: this environment has no running
Doris cluster, so the failure modes are demonstrated at the arithmetic level by
the unit test above rather than through SQL. Happy to add a regression case if
you would prefer one.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
These statements now return the correct rows instead of an empty result or
an exception. Nothing changes for LIMIT and OFFSET values inside the 32-bit
range.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
