mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4101534642
##########
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:
Fixed in 0a52f070b84. The root cause was the FE string -> FLOAT/DOUBLE fold,
so I fixed the fold itself instead of special-casing the level check.
`StringLikeLiteral.castToFloat/castToDouble` now parse the same grammar as BE's
`StringParser::string_to_float` (fast_float):
- The NaN payload form `nan(n-char-seq)` (letters, digits, `_`) is accepted,
case-insensitive and with an optional sign, as in fast_float's `parse_infnan`.
`cast('nan(foo)' as double)` now folds to NaN, not NULL.
- `String.trim()` is gone. The regex captures the number between `\s*` runs.
Java's `\s` is exactly BE's `is_whitespace_ascii` set (space, `\t`, `\n`, `\v`,
`\f`, `\r`), so a value with another control character, such as `'0.25'` plus a
`\x01` byte, now fails on FE as it does on BE. That means NULL under non-strict
cast and an error under strict cast.
`checkLevel()` therefore sees NaN for `'nan(foo)'` and rejects it with
`level must be in [0, 1], but got NaN` in both cast modes. The level it
validates is the value BE executes on the skip-fold and DISTINCT paths.
Coverage:
- `StringLikeLiteralTest` checks both targets, FLOAT and DOUBLE:
- `nan(foo)`, ` -NaN(ind_1) ` and `+nan()` all become NaN.
- All six ASCII whitespace characters around `0.25` are skipped.
- `nan(foo`, `nan(a-b)`, `nan)`, `0.25\u0001`, `\u00010.25`, `1.5d` and
`0x10` all fail.
- `PercentileReservoirParameterTest.testNanPayloadStringLevelIsRejected`
covers `'nan(foo)'` and `cast(' -nan(ind) ' as double)` ×
plain/`_state`/`_combine` × both hooks, under strict and non-strict cast.
- Regression:
- `qt_nan_payload_cast` (FE fold) and `qt_skip_fold_nan_payload_cast` (BE
evaluation) both return `NaN NaN \N`.
- Plain, DISTINCT and `_state` aggregates with both level forms are
rejected, with and without `debug_skip_fold_constant`.
--
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]
