mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4104676703
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnFE.java:
##########
@@ -675,12 +679,34 @@ public Expression visitIf(If ifExpr,
ExpressionRewriteContext context) {
return typeCoercionTrueValue;
} else if (condition.equals(BooleanLiteral.FALSE) ||
condition.isNullLiteral()) {
return typeCoercionFalseValue;
- } else if (typeCoercionTrueValue.equals(typeCoercionFalseValue)) {
+ } else if (isSameBranch(typeCoercionTrueValue,
typeCoercionFalseValue)) {
return typeCoercionTrueValue;
}
return TypeCoercionUtils.ensureSameResultType(originIf, ifExpr,
context);
}
+ // Literal.equals takes NaNs of both signs as equal, but signbit() tells
them apart, so branches
+ // holding a NaN literal, also as an element of a complex literal, are not
merged into one
+ private static boolean isSameBranch(Expression branch, Expression other) {
+ return branch.equals(other) && !branch.anyMatch(expression ->
holdsNaN((Expression) expression));
Review Comment:
Not changed in this PR. The `NULLIF` / OR-dedup / CASE `uniqueOperands`
behavior comes from `Literal.equals`/`hashCode` treating NaNs of both signs as
equal. That is unchanged and gives the same result as at the merge base. There,
`CAST('-nan' AS DOUBLE)` folded to the positive NaN, so both `IF`s in your
example were structurally identical and `NULLIF` already folded to NULL. None
of these rewrites gives a different answer with this PR than without it.
These rewrites also no longer affect `percentile_reservoir`. The level BE
executes is now the literal FE validated, and a NaN level is rejected.
Making literal equality sign-aware for NaN is a global change. Comparing raw
bits breaks, because Java arithmetic produces NaNs whose sign depends on the
operation. It also needs the wire fix from the `DoubleLiteral` thread before a
sign-distinguishing result can survive to BE. So signed-NaN semantics across
the optimizer should be a separate change, not part of this function fix.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/FloatLiteral.java:
##########
@@ -66,6 +66,10 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
return this;
}
if (targetType.isDoubleType()) {
+ if (Float.isNaN(value)) {
+ // widening on BE keeps the sign of a NaN, which signbit() can
observe
+ return new DoubleLiteral(Math.copySign(Double.NaN,
Float.floatToRawIntBits(value) < 0 ? -1.0 : 1.0));
Review Comment:
Not changed in this PR. Even if `FloatLiteral.toLegacyLiteral` kept the
sign, the slot-dependent tree in your example would still lose it at the
fragment wire: Java Thrift `writeDouble` uses `Double.doubleToLongBits`, as
described in the `DoubleLiteral` thread. So fixing only the legacy conversion
would change nothing observable.
This case is also not a regression. At the merge base, `CAST('-nan(foo)' AS
FLOAT)` did not fold at all (NULL under non-strict cast, an error under
strict), while BE returned a negative NaN, so fold on and fold off already
disagreed. The `percentile_reservoir` level is not affected, because BE
executes the FE-validated literal and a NaN level is rejected. Carrying the NaN
sign through FE-to-BE literal transport is a protocol-level change, and it
belongs in a separate PR.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,22 +72,56 @@ private PercentileReservoir(NullableAggregateFunctionParams
functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
+ checkLevel();
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLevel();
+ }
+
+ /**
+ * Execute the level literal that checkLevel() validated. BE would
otherwise evaluate the constant
+ * expression itself wherever it is not folded (load planning, DISTINCT,
debug_skip_fold_constant),
+ * and a cast such as FLOAT to DOUBLE can compute a different value there
than the FE folding.
+ */
+ @Override
+ public Expression rewriteWhenAnalyze() {
+ return withChildren(ImmutableList.of(getArgument(0), checkLevel()));
Review Comment:
Not changed. The representations were already split at the merge base, so no
choice here keeps every existing state compatible:
- At the merge base, `SELECT`, `INSERT ... SELECT` and `INSERT ... VALUES`
states already stored the FE value. `FoldConstantRuleOnFE` folded the coerced
`CAST(<decimal> AS DOUBLE)` with `FractionalLiteral.uncheckedCastTo`, which is
`BigDecimal.doubleValue()` and is not changed by this PR.
- Only load planning (`NereidsLoadUtils` sets `debug_skip_fold_constant`)
let BE compute `(double)unscaled / (double)10^scale`.
So for a level of that precision, states written by queries and states
written by loads could already not be merged with each other before this PR.
The PR makes every writer use one value, the FE value that queries already used.
You are right that there is one upgrade effect. A state written by a load
before the upgrade, with a DECIMAL level of more than about 15 significant
digits, will not merge with one written after it. Keeping the old BE value for
loads would instead keep the query/load split for all new data. The root cause
is that BE's decimal-to-double cast is not correctly rounded, and DECIMAL256
goes through `long double`, so the result depends on the platform. That should
be fixed in BE in a separate PR rather than copied into FE. I'll note the
load-path effect for such high-precision levels in the PR description.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,22 +72,56 @@ private PercentileReservoir(NullableAggregateFunctionParams
functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
+ checkLevel();
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLevel();
Review Comment:
Fixed in 6dd4e45efde. `checkLegalityAfterRewrite` now validates a copy of
the level with every `Nullable` / `NonNullable` replaced by its child
(`rewriteUp`), while the executed level keeps the wrappers, so the requested
state layout is preserved. The pre-push review also found that chained casts
nest the wrappers (`NonNullable(Nullable(0.25))`, or `Cast(Nullable(Cast(0.25
AS FLOAT)) AS DOUBLE)` through a nullable FLOAT level), so every depth is
stripped, not only the top one. The analysis-time check and the level rewrite
still see the unstripped argument, so a user-written `nullable(0.25)` level is
still rejected as at the merge base.
Tests:
`PercentileReservoirParameterTest.testLevelWrappedByAggStateCastIsAccepted`
builds the three shapes with the real `ConvertAggStateCast.convert`, checks
that they are accepted, and checks that `1.5` beneath the wrappers is still
rejected. The regression cases `qt_nullable_level_state_cast`,
`qt_chained_level_state_cast` and `qt_chained_float_level_state_cast` in
`test_percentile_reservoir_constant_level` each return 2.25. Before the fix all
three failed with "must be a constant". `datatype_p0/agg_state` (14 suites)
also passes.
--
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]
