github-actions[bot] commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4101762294
##########
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:
[P2] Do not validate a folded copy and then execute a value that can differ
on BE. For example, use
`CAST((CAST('1.00000005960464483090177623170427978038787841796875' AS FLOAT) >
CAST(1 AS FLOAT)) AS DOUBLE)` as the level. Java parses this decimal directly
to FLOAT and rounds it just above the binary32 midpoint to `0x3f800001`, so
this temporary fold validates level 1. BE's string parser first parses into a
local `double` and only then narrows to FLOAT; that first rounding lands on the
midpoint and tie-to-even narrowing produces `0x3f800000` (1.0), so the retained
comparison executes as level 0. DISTINCT's fold fence and
`debug_skip_fold_constant=true` retain the real child even though
`evaluateWithoutContext` folds this validation copy. Thus the same accepted
aggregate changes from maximum to minimum, and states produced by folded and
retained paths carry incompatible levels. This non-literal comparison was
rejected before this PR, so it is distinct from the pre-existing DECIMAL-
to-DOUBLE thread. Please make the successfully validated value the level
actually executed (or otherwise guarantee FE/BE identity), and add
fold-on/fold-off plus state-merge coverage for a midpoint-adjacent FLOAT case.
##########
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:
[P2] Preserve the sign of a signed NaN payload when folding this cast. A
reduced level is `CAST(signbit(CAST('-nan(foo)' AS DOUBLE)) AS DOUBLE)`. This
branch recognizes the payload but returns canonical positive `Double.NaN`, so
FE folds `signbit(...)` to false and `checkLevel()` validates level 0. The
retained expression differs: [pinned fast_float
v3.9](https://github.com/fastfloat/fast_float/blob/v3.9.0/include/fast_float/parse_number.h#L20-L44)
negates quiet NaN for a leading `-`, and BE's `std::signbit` therefore
produces true/level 1. DISTINCT aggregates and `debug_skip_fold_constant=true`
can retain that tree, changing the aggregate from minimum to maximum; states
made by the two paths then fail the exact-level merge check. This is distinct
from the earlier NULL-vs-NaN payload thread because a raw NaN level is rejected
regardless of sign, while this wrapper turns the sign into two valid opposite
levels. Please preserve the parsed sign for FLOAT and DOUBLE and cover raw bits
plus fold-on/fold-off `signbit(CAST('-nan(payload)' ...))` behavior.
--
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]
