mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4094736743
##########
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:
Fixed in 4f79258. The root cause was the FE fold itself:
`NumericArithmetic.divideDecimal` guarded the dividend instead of the divisor,
so `0 / 2` folded to NULL while BE computed 0 and `1 / 0` was left unfolded
(the level check then reported "must be a constant"). The guard now checks the
divisor, matching `divideDouble`, `divideDecimalV3` and BE, so the value the
legality check validates is the value BE executes and
`debug_skip_fold_constant` no longer changes the result.
Coverage added: `FoldConstantTest` (DECIMALV2 `0 / 2` ->
`DecimalLiteral(0)`, `1 / 0` -> `NullLiteral`),
`PercentileReservoirParameterTest.testDecimalV2DivisionLevelFoldsLikeBe` (`0 /
2`, `3 / 2` rejected at 1.5, `1 / 0`), and the regression suite now runs the
raw DECIMALV2 divisions plus both levels with and without
`debug_skip_fold_constant = true` (`qt_decimalv2_*` /
`qt_skip_fold_decimalv2_*`); both modes produce `0` / `NULL` identically.
##########
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:
Fixed in 4f79258. `checkLevel()` no longer calls
`Literal.checkedCastTo(DOUBLE)` directly. It now wraps the level in the same
implicit cast that signature coercion applies
(`TypeCoercionUtils.castIfNotSameType(level, DOUBLE)`) and folds it with
`FoldConstantRuleOnFE.evaluateWithoutContext`, so
`FoldConstantRuleOnFE.visitCast` applies the session cast mode: a non-strict
failure becomes a `NullLiteral` (accepted, NULL result) and a strict failure
still throws. `percentile_reservoir(number, '')` and `cast('' as double)`
therefore agree again in both modes.
Coverage added:
`PercentileReservoirParameterTest.testInvalidStringLevelFollowsImplicitCastMode`
(`''`, `'abc'`, `cast('' as double)` x plain/`_state`/`_combine` x both hooks,
under `enable_strict_cast` false and true) and the regression suite runs both
forms in both cast modes, also with `debug_skip_fold_constant = true`
(`qt_empty_string_level`, `qt_empty_string_cast_level`,
`qt_skip_fold_empty_string_*` and the strict-mode `test{}` blocks).
--
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]
