mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4103870644
##########
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()
Review Comment:
Thanks, confirmed and fixed in a0e4f5b2331 and 5495a8602c3.
Reproduced on a local cluster before the fix: with
`CAST((CAST('1.00000005960464483090177623170427978038787841796875' AS FLOAT) >
CAST(1 AS FLOAT)) AS DOUBLE)` as the level, `percentile_reservoir` over
`numbers(10)` returned 9 with folding and 0 with `debug_skip_fold_constant =
true`. BE `StringParser::string_to_float_internal` parses into a `double` and
then narrows, while FE used `Float.parseFloat`.
- a0e4f5b2331: FE casts a string to FLOAT by parsing a double and narrowing
it (`StringLikeLiteral.castToFloat`). It also narrows DOUBLE to FLOAT with a
single rounding instead of going through the shortest decimal string: the
double 1 + 2^-24 prints as `1.0000000596046448`, which is above the midpoint,
so `cast(cast('1.0000000596046448' as double) as float)` had the same problem.
- 5495a8602c3 does what you suggested and makes the validated value the
executed one. `PercentileReservoir` implements `RewriteWhenAnalyze`, so once
the function is analyzed, the level is replaced with the literal `checkLevel()`
validated. `StateCombinator` and `CombineCombinator` forward this to their
nested function. BE therefore executes the checked literal on every path:
plain, DISTINCT, window, `_state` / `_combine`, `INSERT ... VALUES`, and load
column mappings, which always plan with constant folding off. That also covers
casts whose FE folding still differs from BE, e.g. `cast(0.1 as float)`: FE
widens it to 0.1, BE to 0.10000000149011612. Before this change, a stream load
mapping `s = percentile_reservoir_state(v, cast(0.1 as float))` wrote a state
that failed with "incompatible quantiles" when merged with a state built by a
query.
Tests:
- UT:
- `StringLikeLiteralTest` and `DoubleLiteralTest` pin the raw bits.
-
`PercentileReservoirParameterTest.testAnalyzedLevelIsTheValidatedLiteral`
checks that the level is replaced for the function, `_state` and `_combine`.
- Regression (`test_percentile_reservoir_constant_level`):
- the three midpoint and signed-NaN level shapes, plus `cast(0.1 as
float)`, give identical results with and without folding, for plain, DISTINCT
and window aggregation;
- a stored FE-folded state merges with a BE-computed one;
- a stream load state merges with a query-built state.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StringLikeLiteral.java:
##########
@@ -198,39 +200,46 @@ protected Expression castToIntegral(DataType targetType,
boolean strictCast) {
}
protected Expression castToFloat() {
- String trimmedValue = value.trim();
- if (doublePattern.matcher(trimmedValue).matches()) {
+ Matcher matcher = doublePattern.matcher(value);
+ if (matcher.matches()) {
+ String trimmedValue = matcher.group("number");
if
(DoubleLiteral.POS_INF_NAME.contains(trimmedValue.toLowerCase())) {
return Literal.of(Float.POSITIVE_INFINITY);
}
if
(DoubleLiteral.NEG_INF_NAME.contains(trimmedValue.toLowerCase())) {
return Literal.of(Float.NEGATIVE_INFINITY);
}
- if (DoubleLiteral.NAN_NAME.contains(trimmedValue.toLowerCase())) {
+ if
(DoubleLiteral.NAN_NAME.contains(trimNanPayload(trimmedValue).toLowerCase())) {
return Literal.of(Float.NaN);
}
- return Literal.of(Float.parseFloat(value.trim()));
+ return Literal.of(Float.parseFloat(trimmedValue));
}
throw new CastException(String.format("%s can't cast to float in
strict mode.", value));
}
protected Expression castToDouble() {
- String trimmedValue = value.trim();
- if (doublePattern.matcher(trimmedValue).matches()) {
+ Matcher matcher = doublePattern.matcher(value);
+ if (matcher.matches()) {
+ String trimmedValue = matcher.group("number");
if
(DoubleLiteral.POS_INF_NAME.contains(trimmedValue.toLowerCase())) {
return Literal.of(Double.POSITIVE_INFINITY);
}
if
(DoubleLiteral.NEG_INF_NAME.contains(trimmedValue.toLowerCase())) {
return Literal.of(Double.NEGATIVE_INFINITY);
}
- if (DoubleLiteral.NAN_NAME.contains(trimmedValue.toLowerCase())) {
+ if
(DoubleLiteral.NAN_NAME.contains(trimNanPayload(trimmedValue).toLowerCase())) {
Review Comment:
Thanks, confirmed and fixed in a0e4f5b2331 and 5495a8602c3.
Before the fix, `signbit(cast('-nan(foo)' as double))` folded to false on FE
and returned true on BE, so `CAST(signbit(CAST('-nan(foo)' AS DOUBLE)) AS
DOUBLE)` as the level returned the minimum with folding and the maximum with
`debug_skip_fold_constant = true`.
- a0e4f5b2331: `StringLikeLiteral` folds `-nan` / `-nan(payload)` to a
negative NaN, matching fast_float's `parse_infnan`. The FLOAT form keeps the
sign too, and FLOAT<->DOUBLE literal casts preserve the NaN sign, as
`static_cast` does on BE.
- 5495a8602c3: the level is now executed as the literal FE validated
(`RewriteWhenAnalyze`, also forwarded by the `_state` / `_combine`
combinators), so the two paths can no longer run different levels.
- Signed NaNs exposed one more fold that relied on NaN equality:
`FoldConstantRuleOnFE` merged If / CASE branches that are `equals`, and
`Literal.equals` treats NaNs of both signs as equal. So `if(<unfoldable false>,
cast('-nan' as double), cast('nan' as double))` collapsed to the true branch.
- Branches that hold a FLOAT/DOUBLE NaN literal, including elements of an
array, map or struct literal, are now never merged. Such a level stays unfolded
and is rejected as not constant, as it was before this PR.
Tests:
- UT:
- `StringLikeLiteralTest` asserts the raw bits: `0xfff8000000000000` /
`0xffc00000` for `-nan`, positive for `nan(foo)` / `+nan`, and the sign kept
through the FLOAT->DOUBLE cast.
- `FoldConstantTest` checks that If / CASE over -NaN/+NaN DOUBLE, FLOAT
and ARRAY literals is not collapsed.
- Regression: `signbit` of `-nan(foo)`, `' -nan '`, `nan(foo)` and FLOAT
`-nan(foo)` gives the same `true true false true` with and without folding; the
signed-NaN level gives the same result for plain and DISTINCT in both modes;
the NaN-If level shapes are rejected.
--
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]
