[Fix](topn) Reject non-positive topn count argument#63350
Open
linrrzqqq wants to merge 2 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
previously approved these changes
May 18, 2026
Contributor
|
/review |
Collaborator
Author
|
run buildall |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking issue. The new validation is in the right area for Nereids and the added negative/zero coverage is useful, but it runs after signature coercion to IntegerType. That makes an existing positive out-of-range literal test fail as a non-positive value after Java int narrowing.
Critical checkpoint conclusions:
- Goal/test: partially accomplished for negative and zero Nereids topn counts, but existing topn_weighted type-coercion coverage is not updated and appears to regress.
- Scope/focus: the change is small and focused.
- Concurrency/lifecycle/config/compatibility/transaction/write paths: not involved.
- Parallel paths: no active original-planner path found; Nereids path is the relevant path.
- Conditional checks: the new condition needs to account for post-coercion narrowing/out-of-range literals.
- Test coverage/results: added tests cover -1 and 0, but an existing regression test with 2147483648 is now impacted and no expected result/error update is included.
- Observability/performance: not applicable.
User focus: no additional user-provided focus points were supplied.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 31554 ms |
Contributor
TPC-DS: Total hot run time: 169217 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Collaborator
Author
|
/review |
Collaborator
Author
|
run buildall |
github-actions
Bot
removed
the
approved
Contributor
TPC-H: Total hot run time: 31381 ms |
Contributor
There was a problem hiding this comment.
I found blocking issues in the actual PR diff. The existing review thread already covers the TopNWeighted overflow/regression-test mismatch, so I did not duplicate it. The same post-coercion overflow problem exists in the newly added TopN and TopNArray checks as distinct instances.
Critical checkpoint conclusions:
- Goal and tests: rejecting non-positive TopN counts is reasonable, but the implementation does not correctly validate out-of-range positive literals before IntegerType coercion; existing coverage only adds -1/0 cases.
- Scope/focus: the actual PR diff is small and focused on TopN count validation.
- Concurrency/lifecycle/configuration/persistence/storage format: not applicable.
- Compatibility/parallel paths: the same validation pattern was added to all three TopN variants;
TopNWeightedis already reported, andTopN/TopNArrayneed the same fix. - Test coverage: missing positive out-of-range count cases for
topnandtopn_array; the already-knowntopn_weightedexisting regression also needs to be updated or preserved correctly. - Observability: not applicable.
User focus: no additional user-provided focus points were supplied.
| } | ||
| if (!(topNCount instanceof Literal) || ((Literal) topNCount).getDouble() <= 0) { | ||
| throw new AnalysisException( | ||
| "topn requires second parameter must be a constant positive integer: " |
Contributor
There was a problem hiding this comment.
This has the same post-coercion overflow problem as the already reported TopNWeighted case, but for topn. The second argument is coerced to IntegerType before checkLegalityAfterRewrite, and literal coercion uses Number.intValue(), so topn(col, 2147483648) reaches this check as IntegerLiteral(-2147483648) and is reported as a non-positive value instead of being handled as a positive out-of-range count. Please validate the original literal range before narrowing, or explicitly reject out-of-range counts with coverage for topn.
| } | ||
| if (!(topNCount instanceof Literal) || ((Literal) topNCount).getDouble() <= 0) { | ||
| throw new AnalysisException( | ||
| "topn_array requires second parameter must be a constant positive integer: " |
Contributor
There was a problem hiding this comment.
This is a distinct instance of the same overflow bug in topn_array. Because the count argument is coerced to IntegerType before this check, a positive literal such as 2147483648 is narrowed to a negative IntegerLiteral and then rejected as non-positive. Please validate/reject the original literal range before the narrowing cast, and add regression coverage for topn_array positive out-of-range counts.
Contributor
TPC-DS: Total hot run time: 170689 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Summary:
According to Doris doc:
topn count must be a positive integer, but before this, negative numbers and 0 were not handled specially, returning{}, leading to unexpected results.before
now: