github-actions[bot] commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4091861127
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,21 +69,38 @@ private PercentileReservoir(NullableAggregateFunctionParams
functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
+ checkLevel();
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLevel();
+ }
+
+ /**
+ * The level must be a constant that folds to a literal in [0, 1]. It is
folded here instead of
+ * waiting for the rewrite phase because a constant expression such as
0.25 + 0.25 is only a
+ * literal after folding, some plans (INSERT ... VALUES, load column
mappings) never run the
+ * rewrite phase, and constant folding can be turned off by
debug_skip_fold_constant.
+ */
+ private void checkLevel() {
Expression levelArgument = getArgument(1);
- if (!levelArgument.isConstant()) {
+ Expression level = levelArgument.isConstant()
+ ? FoldConstantRuleOnFE.evaluateWithoutContext(levelArgument) :
levelArgument;
+ if (!(level instanceof Literal)) {
throw new AnalysisException(
"percentile_reservoir requires second parameter must be a
constant : " + this.toSql());
}
- if (levelArgument instanceof Literal) {
- double value = ((Literal) levelArgument).getDouble();
- // Negate the valid range to reject NaN, which makes both < 0 and
> 1 false.
- if (!(value >= 0 && value <= 1)) {
- throw new AnalysisException(
- "percentile_reservoir level must be in [0, 1], but got
" + value + ": " + this.toSql());
- }
- } else {
+ // a NULL level is skipped by the null-ignoring BE implementation and
yields a NULL result
+ if (level instanceof NullLiteral) {
+ return;
+ }
+ // the literal may still carry its own type here, for example DECIMAL
or VARCHAR
+ double value = ((Literal) ((Literal)
level).checkedCastTo(DoubleType.INSTANCE)).getDouble();
Review Comment:
[P2] Preserve non-strict implicit-cast semantics here. With the default
`enable_strict_cast=false`, this direct `checkedCastTo(DOUBLE)` makes
`percentile_reservoir(number, '')` throw before signature coercion. In the base
code the empty string passed the old range check and the ordinary
VARCHAR-to-DOUBLE cast then produced NULL; the equivalent explicit `cast('' as
double)` still produces NULL and is accepted above. This therefore introduces a
syntax-dependent regression for equivalent implicit and explicit casts. Please
range-check the result of the same session-aware DOUBLE cast/fold used by
normal coercion (so non-strict failures become `NullLiteral` and strict
failures still throw), and cover both forms in both cast modes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,21 +69,38 @@ private PercentileReservoir(NullableAggregateFunctionParams
functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
+ checkLevel();
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLevel();
+ }
+
+ /**
+ * The level must be a constant that folds to a literal in [0, 1]. It is
folded here instead of
+ * waiting for the rewrite phase because a constant expression such as
0.25 + 0.25 is only a
+ * literal after folding, some plans (INSERT ... VALUES, load column
mappings) never run the
+ * rewrite phase, and constant folding can be turned off by
debug_skip_fold_constant.
+ */
+ private void checkLevel() {
Expression levelArgument = getArgument(1);
- if (!levelArgument.isConstant()) {
+ Expression level = levelArgument.isConstant()
+ ? FoldConstantRuleOnFE.evaluateWithoutContext(levelArgument) :
levelArgument;
Review Comment:
[P1] Do not validate this level against a different DECIMALV2 result than
BE. For `percentile_reservoir(number, cast(0 as decimalv2(27,9)) / cast(2 as
decimalv2(27,9)))`, this helper calls `NumericArithmetic.divideDecimal`, whose
zero guard checks the numerator and folds `0 / 2` to NULL. Normal rewrite
repeats that fold, so the aggregate returns NULL; with
`debug_skip_fold_constant=true`, this check still accepts NULL but the original
expression reaches BE, whose denominator check computes level 0, so the
aggregate returns the minimum value. The setting therefore changes query
results and the legality check validates a value BE does not execute. Please
fix the FE DECIMALV2 denominator check (including `1 / 0`) or otherwise avoid
relying on the mismatched fold, and cover both folding modes.
--
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]
