From 31ce20b150e60e67ffbe71a19dc50fcd0ebd28bd Mon Sep 17 00:00:00 2001 From: HappenLee Date: Thu, 16 Jan 2025 11:53:56 +0800 Subject: [PATCH] [Fix](bug) Percentile* func core when percent args is negative number --- .../expressions/functions/agg/Percentile.java | 26 +++++++++++--- .../functions/agg/PercentileApprox.java | 27 ++++++++++++--- .../agg/PercentileApproxWeighted.java | 27 ++++++++++++--- .../functions/agg/PercentileArray.java | 33 ++++++++++++++++++ .../query_p0/aggregate/aggregate.groovy | 17 ++++++++++ .../test_aggregate_all_functions.groovy | 34 +++++++++++++++++++ 6 files changed, 149 insertions(+), 15 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Percentile.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Percentile.java index 17f501b7f1333aa..239b5bed2c46d31 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Percentile.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Percentile.java @@ -21,6 +21,7 @@ import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; import org.apache.doris.nereids.trees.expressions.shape.BinaryExpression; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.BigIntType; @@ -50,7 +51,6 @@ public class Percentile extends NullableAggregateFunction FunctionSignature.ret(DoubleType.INSTANCE).args(IntegerType.INSTANCE, DoubleType.INSTANCE), FunctionSignature.ret(DoubleType.INSTANCE).args(SmallIntType.INSTANCE, DoubleType.INSTANCE), FunctionSignature.ret(DoubleType.INSTANCE).args(TinyIntType.INSTANCE, DoubleType.INSTANCE) - ); /** @@ -79,6 +79,22 @@ public void checkLegalityBeforeTypeCoercion() { } } + @Override + public void checkLegalityAfterRewrite() { + Expression arg1 = getArgument(1); + if (!(arg1 instanceof DoubleLiteral)) { + throw new AnalysisException( + "percentile requires second parameter must be a constant double: " + this.toSql()); + } + DoubleLiteral data = (DoubleLiteral) arg1; + double realData = data.getValue(); + if (realData < 0 || realData > 1) { + throw new AnalysisException( + "percentile requires second parameter must be a constant double in [0, 1]: " + this.toSql() + + " but the value is " + realData); + } + } + /** * withDistinctAndChildren. */ @@ -89,13 +105,13 @@ public Percentile withDistinctAndChildren(boolean distinct, List chi } @Override - public NullableAggregateFunction withAlwaysNullable(boolean alwaysNullable) { - return new Percentile(distinct, alwaysNullable, children.get(0), children.get(1)); + public R accept(ExpressionVisitor visitor, C context) { + return visitor.visitPercentile(this, context); } @Override - public R accept(ExpressionVisitor visitor, C context) { - return visitor.visitPercentile(this, context); + public NullableAggregateFunction withAlwaysNullable(boolean alwaysNullable) { + return new Percentile(distinct, alwaysNullable, children.get(0), children.get(1)); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApprox.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApprox.java index e4d7dc66c84ec5d..5fc7c052c8a7923 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApprox.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApprox.java @@ -21,6 +21,7 @@ import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.DoubleType; @@ -92,6 +93,22 @@ public void checkLegalityBeforeTypeCoercion() { } } + @Override + public void checkLegalityAfterRewrite() { + Expression arg1 = getArgument(1); + if (!(arg1 instanceof DoubleLiteral)) { + throw new AnalysisException( + "percentile_approx requires second parameter must be a constant double: " + this.toSql()); + } + DoubleLiteral data = (DoubleLiteral) arg1; + double realData = data.getValue(); + if (realData < 0 || realData > 1) { + throw new AnalysisException( + "percentile_approx requires second parameter must be a constant double in [0, 1]: " + this.toSql() + + " but the value is " + realData); + } + } + /** * withDistinctAndChildren. */ @@ -106,6 +123,11 @@ public PercentileApprox withDistinctAndChildren(boolean distinct, List R accept(ExpressionVisitor visitor, C context) { + return visitor.visitPercentileApprox(this, context); + } + @Override public PercentileApprox withAlwaysNullable(boolean alwaysNullable) { if (children.size() == 2) { @@ -115,11 +137,6 @@ public PercentileApprox withAlwaysNullable(boolean alwaysNullable) { } } - @Override - public R accept(ExpressionVisitor visitor, C context) { - return visitor.visitPercentileApprox(this, context); - } - @Override public List getSignatures() { return SIGNATURES; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxWeighted.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxWeighted.java index 8698ee8af153525..333f109f501fabb 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxWeighted.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxWeighted.java @@ -21,6 +21,7 @@ import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.DoubleType; @@ -97,6 +98,22 @@ public void checkLegalityBeforeTypeCoercion() { } } + @Override + public void checkLegalityAfterRewrite() { + Expression arg2 = getArgument(2); + if (!(arg2 instanceof DoubleLiteral)) { + throw new AnalysisException( + "percentile_approx_weighted requires third parameter must be a constant double: " + this.toSql()); + } + DoubleLiteral data = (DoubleLiteral) arg2; + double realData = data.getValue(); + if (realData < 0 || realData > 1) { + throw new AnalysisException( + "percentile_approx_weighted requires third parameter must be a constant double in [0, 1]: " + + this.toSql() + " but the value is " + realData); + } + } + /** * withDistinctAndChildren. */ @@ -113,6 +130,11 @@ public PercentileApproxWeighted withDistinctAndChildren(boolean distinct, } } + @Override + public R accept(ExpressionVisitor visitor, C context) { + return visitor.visitPercentileApprox(this, context); + } + @Override public PercentileApproxWeighted withAlwaysNullable(boolean alwaysNullable) { if (children.size() == 3) { @@ -124,11 +146,6 @@ public PercentileApproxWeighted withAlwaysNullable(boolean alwaysNullable) { } } - @Override - public R accept(ExpressionVisitor visitor, C context) { - return visitor.visitPercentileApprox(this, context); - } - @Override public List getSignatures() { return SIGNATURES; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileArray.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileArray.java index 1abbe4d54505314..586f51e4f278d9f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileArray.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileArray.java @@ -18,9 +18,12 @@ package org.apache.doris.nereids.trees.expressions.functions.agg; import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; import org.apache.doris.nereids.trees.expressions.literal.ArrayLiteral; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; +import org.apache.doris.nereids.trees.expressions.literal.Literal; import org.apache.doris.nereids.trees.expressions.shape.BinaryExpression; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.ArrayType; @@ -74,6 +77,36 @@ public PercentileArray(boolean distinct, Expression arg0, Expression arg1) { super("percentile_array", distinct, arg0, arg1); } + @Override + public void checkLegalityBeforeTypeCoercion() { + if (!getArgument(1).isConstant()) { + throw new AnalysisException( + "percentile_array requires second parameter must be a constant : " + this.toSql()); + } + } + + @Override + public void checkLegalityAfterRewrite() { + Expression arg1 = getArgument(1); + if (!(arg1 instanceof ArrayLiteral)) { + throw new AnalysisException( + "percentile_approx requires second parameter must be a constant array: " + this.toSql()); + } + ArrayLiteral data = (ArrayLiteral) arg1; + for (Literal d : data.getValue()) { + if (!(d instanceof DoubleLiteral)) { + throw new AnalysisException( + "percentile_array requires second parameter must be a constant array[double]: " + this.toSql()); + } + double realData = ((DoubleLiteral) d).getValue(); + if (realData < 0 || realData > 1) { + throw new AnalysisException( + "percentile_array requires second parameter must be a constant array[double], " + + "double value in [0, 1]: " + this.toSql() + " but the value is " + realData); + } + } + } + /** * withDistinctAndChildren. */ diff --git a/regression-test/suites/query_p0/aggregate/aggregate.groovy b/regression-test/suites/query_p0/aggregate/aggregate.groovy index b611ff92b0eabaa..94e0cecf9d4aa1d 100644 --- a/regression-test/suites/query_p0/aggregate/aggregate.groovy +++ b/regression-test/suites/query_p0/aggregate/aggregate.groovy @@ -141,6 +141,23 @@ suite("aggregate") { qt_aggregate32" select topn_weighted(c_string,c_bigint,3) from ${tableName}" qt_aggregate33" select avg_weighted(c_double,c_bigint) from ${tableName};" qt_aggregate34" select percentile_array(c_bigint,[0.2,0.5,0.9]) from ${tableName};" + + try { + sql "select percentile_array(c_bigint,[-1,0.5,0.9]) from ${tableName};" + } catch (Exception ex) { + assert("${ex}".contains("-1")) + } + try { + sql "select percentile_array(c_bigint,[0.5,0.9,3000]) from ${tableName};" + } catch (Exception ex) { + assert("${ex}".contains("3000")) + } + try { + sql "select percentile_array(c_bigint,[0.5,0.9,null]) from ${tableName};" + } catch (Exception ex) { + assert("${ex}".contains("double")) + } + qt_aggregate """ SELECT c_bigint, CASE diff --git a/regression-test/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions.groovy b/regression-test/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions.groovy index cdab9472e27dbd1..f3eb2c653d82db8 100644 --- a/regression-test/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions.groovy +++ b/regression-test/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions.groovy @@ -287,6 +287,23 @@ suite("test_aggregate_all_functions", "arrow_flight_sql") { qt_select21_1 "select id,percentile(level + 0.1,0.55) from ${tableName_13} group by id order by id" qt_select22_1 "select id,percentile(level + 0.1,0.805) from ${tableName_13} group by id order by id" + try { + sql "select id,percentile(level + 0.1, -1) from ${tableName_13} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("-1")) + } + try { + sql "select id,percentile(level + 0.1, 3000) from ${tableName_13} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("3000")) + } + try { + sql "select id,percentile(level + 0.1, null) from ${tableName_13} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("double")) + } + + sql "DROP TABLE IF EXISTS ${tableName_13}" @@ -314,6 +331,23 @@ suite("test_aggregate_all_functions", "arrow_flight_sql") { qt_select27 "select id,PERCENTILE_APPROX(level,0.55,2048) from ${tableName_14} group by id order by id" qt_select28 "select id,PERCENTILE_APPROX(level,0.805,2048) from ${tableName_14} group by id order by id" + try { + sql "select id,PERCENTILE_APPROX(level, -1, 2048) from ${tableName_14} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("-1")) + } + try { + sql "select id,PERCENTILE_APPROX(level, 3000 ,2048) from ${tableName_14} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("3000")) + } + try { + sql "select id,PERCENTILE_APPROX(level, null ,2048) from ${tableName_14} group by id order by id" + } catch (Exception ex) { + assert("${ex}".contains("double")) + } + + sql "DROP TABLE IF EXISTS ${tableName_14}"