github-actions[bot] commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4101297062
##########
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 string cast identical to the value BE executes. A
reduced path is:
```text
Aggregate(percentile_reservoir(v, Cast('nan(foo)' AS DOUBLE)))
```
`checkLevel()` folds only a temporary cast. FE's DOUBLE regex rejects the
payload form, and non-strict `visitCast` turns that failure into `NullLiteral`,
so the level is accepted here. The actual coerced child is unchanged, though:
with `debug_skip_fold_constant=true`, or by default for a DISTINCT aggregate
(whose whole expression `FoldConstantRuleOnFE` skips), BE's pinned fast_float
parser accepts `nan(foo)` as a non-NULL NaN. That NaN bypasses the `[0,1]`
check and reaches `QuantileReservoirSampler`; nonempty partial states then fail
its exact level comparison because NaN is unequal to itself. This is distinct
from the existing `''`/`abc` thread, where both parsers fail and NULL is the
actual runtime value. Please align FE validation with BE's string-to-DOUBLE
token/trimming grammar (the inverse mismatch also exists for trailing controls
Java `trim()` drops), and cover ordinary, skip-fold, and DISTINCT/state paths.
--
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]
