github-actions[bot] commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4100436496
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,21 +70,42 @@ 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.
+ * The level is brought to DOUBLE with the same implicit cast that
signature coercion applies,
+ * so a level that is not a valid DOUBLE behaves like the coerced
expression: NULL under the
+ * default non-strict cast and an error under strict cast, for '' as well
as cast('' as double).
+ */
+ private void checkLevel() {
Expression levelArgument = getArgument(1);
- if (!levelArgument.isConstant()) {
+ Expression level = levelArgument.isConstant()
+ ? FoldConstantRuleOnFE.evaluateWithoutContext(
Review Comment:
[P2] Keep the validated DOUBLE identical to the value BE executes. A reduced
triggering tree is:
```text
Aggregate(percentile_reservoir(v,
Cast(Divide(Cast(d1 AS DECIMAL(35,34)), d2) AS DOUBLE)))
```
With default `div_precision_increment`, `d1 = cast(1 as decimalv3(14,13))`
and `d2 = cast(0.99999999999999991 as decimalv3(18,17))` produce DECIMAL(35,17)
`1.00000000000000009` (unscaled `100000000000000009`). This check folds the
cast via `BigDecimal.doubleValue()` to `1.0` and accepts it. With
`debug_skip_fold_constant=true`, the retained cast reaches BE, whose decimal
cast computes `(double)100000000000000009 / (double)10^17 =
1.0000000000000002`. States persisted by sessions using normal and skipped
folding therefore serialize different levels, and
`QuantileReservoirSampler::merge` rejects them as incompatible. This is
distinct from the resolved exact-division thread: the decimal quotient is now
correct; the following DECIMAL-to-DOUBLE conversion diverges. Please make FE
validation/folding use BE-equivalent conversion and cover this scale-17
boundary in both modes, including merging states produced by each path.
--
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]
