Skip to content

Commit f318357

Browse files
linrrzqqqlinrrzqqq
authored
[Fix](topn) Reject non-positive topn count argument (#63350)
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 ```sql Doris> SELECT TOPN(page_id, 0) as top_zero_pages FROM page_visits; +----------------+ | top_zero_pages | +----------------+ | {} | +----------------+ Doris> SELECT TOPN(page_id, -1) as top_neg_one_pages FROM page_visits; +-------------------+ | top_neg_one_pages | +-------------------+ | {} | +-------------------+ ``` now: ```sql Doris> SELECT TOPN(page_id, 0) as top_pages FROM page_visits; ERROR 1105 (HY000): errCode = 2, detailMessage = topn requires second parameter must be a constant positive integer: topn(cast(page_id as VARCHAR(65533)), 0) Doris> SELECT TOPN(page_id, -1) as top_pages FROM page_visits; ERROR 1105 (HY000): errCode = 2, detailMessage = topn requires second parameter must be a constant positive integer: topn(cast(page_id as VARCHAR(65533)), -1) ```
1 parent 5510743 commit f318357

4 files changed

Lines changed: 74 additions & 0 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopN.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.doris.nereids.exceptions.AnalysisException;
2222
import org.apache.doris.nereids.trees.expressions.Expression;
2323
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
24+
import org.apache.doris.nereids.trees.expressions.literal.Literal;
2425
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
2526
import org.apache.doris.nereids.types.IntegerType;
2627
import org.apache.doris.nereids.types.VarcharType;
@@ -97,6 +98,19 @@ public void checkLegalityBeforeTypeCoercion() {
9798
}
9899
}
99100

101+
@Override
102+
public void checkLegalityAfterRewrite() {
103+
Expression topNCount = getArgument(1);
104+
if (topNCount.isNullLiteral()) {
105+
return;
106+
}
107+
if (!(topNCount instanceof Literal) || ((Literal) topNCount).getDouble() <= 0) {
108+
throw new AnalysisException(
109+
"topn requires second parameter must be a constant positive integer: "
110+
+ this.toSql());
111+
}
112+
}
113+
100114
/**
101115
* withDistinctAndChildren.
102116
*/

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopNArray.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.doris.nereids.exceptions.AnalysisException;
2222
import org.apache.doris.nereids.trees.expressions.Expression;
2323
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
24+
import org.apache.doris.nereids.trees.expressions.literal.Literal;
2425
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
2526
import org.apache.doris.nereids.types.ArrayType;
2627
import org.apache.doris.nereids.types.IntegerType;
@@ -100,6 +101,19 @@ public void checkLegalityBeforeTypeCoercion() {
100101
}
101102
}
102103

104+
@Override
105+
public void checkLegalityAfterRewrite() {
106+
Expression topNCount = getArgument(1);
107+
if (topNCount.isNullLiteral()) {
108+
return;
109+
}
110+
if (!(topNCount instanceof Literal) || ((Literal) topNCount).getDouble() <= 0) {
111+
throw new AnalysisException(
112+
"topn_array requires second parameter must be a constant positive integer: "
113+
+ this.toSql());
114+
}
115+
}
116+
103117
/**
104118
* withDistinctAndChildren.
105119
*/

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopNWeighted.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.doris.nereids.exceptions.AnalysisException;
2222
import org.apache.doris.nereids.trees.expressions.Expression;
2323
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
24+
import org.apache.doris.nereids.trees.expressions.literal.Literal;
2425
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
2526
import org.apache.doris.nereids.types.ArrayType;
2627
import org.apache.doris.nereids.types.BigIntType;
@@ -205,4 +206,17 @@ public void checkLegalityBeforeTypeCoercion() {
205206
+ this.toSql());
206207
}
207208
}
209+
210+
@Override
211+
public void checkLegalityAfterRewrite() {
212+
Expression topNCount = getArgument(2);
213+
if (topNCount.isNullLiteral()) {
214+
return;
215+
}
216+
if (!(topNCount instanceof Literal) || ((Literal) topNCount).getDouble() <= 0) {
217+
throw new AnalysisException(
218+
"topn_weighted requires third parameter must be a constant positive integer: "
219+
+ this.toSql());
220+
}
221+
}
208222
}

‎regression-test/suites/nereids_function_p0/agg_function/topn/topn.groovy‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,21 +51,53 @@ suite("topn") {
5151
exception "errCode = 2"
5252
}
5353

54+
test {
55+
sql """select topn(id,-1) from test_topn;"""
56+
exception "topn requires second parameter must be a constant positive integer"
57+
}
58+
59+
test {
60+
sql """select topn(id,0) from test_topn;"""
61+
exception "topn requires second parameter must be a constant positive integer"
62+
}
63+
5464
test {
5565
sql """select topn_array(id,id) from test_topn;"""
5666
exception "errCode = 2"
5767
}
68+
5869
test {
5970
sql """select topn_array(id,1,id) from test_topn;"""
6071
exception "errCode = 2"
6172
}
6273

74+
test {
75+
sql """select topn_array(id,-1) from test_topn;"""
76+
exception "topn_array requires second parameter must be a constant positive integer"
77+
}
78+
79+
test {
80+
sql """select topn_array(id,0) from test_topn;"""
81+
exception "topn_array requires second parameter must be a constant positive integer"
82+
}
83+
6384
test {
6485
sql """select topn_weighted(id,id,id) from test_topn;"""
6586
exception "errCode = 2"
6687
}
88+
6789
test {
6890
sql """select topn_weighted(id,id,1,id) from test_topn;"""
6991
exception "errCode = 2"
7092
}
93+
94+
test {
95+
sql """select topn_weighted(id,id,-1) from test_topn;"""
96+
exception "topn_weighted requires third parameter must be a constant positive integer"
97+
}
98+
99+
test {
100+
sql """select topn_weighted(id,id,0) from test_topn;"""
101+
exception "topn_weighted requires third parameter must be a constant positive integer"
102+
}
71103
}

0 commit comments

Comments
 (0)