github-actions[bot] commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4104200791
##########
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:
[P2] Make this signed-NaN distinction apply to every rewrite that treats
structural equality as semantic equality. This private helper fixes only
If/CASE result branches; `Literal.equals`/`hashCode` still canonicalize
opposite NaN signs. For example, `NULLIF(signbit(IF(number=0, CAST('nan' AS
DOUBLE), CAST(0 AS DOUBLE))), signbit(IF(number=0, CAST('-nan' AS DOUBLE),
CAST(0 AS DOUBLE))))` is folded to NULL by `first.equals(second)`, but at
`number=0` BE evaluates FALSE versus TRUE and returns FALSE. The same
predicates under OR are deduplicated by `DistinctPredicatesRule` to the false
first operand although the OR is true, and CASE `uniqueOperands` has the same
problem. These are distinct from the resolved branch-result thread. Please use
a shared raw-bit-aware equivalence (or conservatively skip equality rewrites
for expressions holding NaNs) and cover dynamic NULLIF/OR/CASE cases.
##########
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:
[P2] Accept the planner's safe nullable-layout wrapper in this post-rewrite
check. `CAST(percentile_reservoir_state(CAST(7 AS DOUBLE), 0.25) AS
AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>)` was valid at
the merge base. `ConvertAggStateCast` retargets the level subtype by wrapping
`0.25` in `Nullable`, and `StateCombinator` forwards that child here;
`evaluateWithoutContext` deliberately leaves `Nullable` intact, so this new
literal-only check rejects it as "must be a constant" even though BE can safely
materialize the non-null literal in a nullable column. Please validate the
non-null literal beneath this wrapper while preserving the requested state
layout, or avoid reapplying the literal-only check to this converted child, and
add a full-rewrite test that makes the level subtype nullable.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/DoubleLiteral.java:
##########
@@ -86,8 +86,10 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
return this;
}
if (targetType.isFloatType()) {
- return new
org.apache.doris.nereids.trees.expressions.literal.FloatLiteral(
- Float.parseFloat(String.valueOf(value)));
+ // narrow with a single rounding and keep the sign of a NaN, like
static_cast<float> on BE
Review Comment:
[P2] Preserve this sign across the fragment wire format, not only inside the
literal objects. In `signbit(IF(number=0, CAST('-nan' AS DOUBLE), 0.0))`,
folding creates a negative `DoubleLiteral` but leaves the slot-dependent IF.
The legacy literal and `TFloatLiteral.value` still hold that sign, but the
pinned Java Thrift binary/compact `writeDouble` paths use
`Double.doubleToLongBits`, which canonicalizes every NaN to the positive
pattern before BE receives the fragment. Normal folding therefore returns false
at `number=0`, while `debug_skip_fold_constant` lets BE parse `-nan` and
returns true. This is distinct from the FLOAT `toLegacyLiteral` issue: fixing
that earlier conversion does not fix this DOUBLE wire boundary. Please retain a
BE-evaluated sign-producing expression or use a compatibility-safe raw-bit
representation, and add dynamic fold-on/off fragment-serialization coverage.
##########
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:
[P2] Preserve compatibility with states serialized before this rewrite. The
merge base already accepts a DECIMALV3 level such as `0.12345678901234567`; its
retained/load path lets BE compute `(double)12345678901234567 / (double)10^17`
(`0x3fbf9add3746f65f`), while this analyzer rewrite now executes
`BigDecimal.doubleValue()`'s literal (`0x3fbf9add3746f65e`). Both states keep
the same AggState type, but `QuantileReservoirSampler` serializes the raw level
and rejects an exact mismatch when nonempty states merge. Existing persisted
load-created rows or mixed-version writers can therefore start failing after
upgrade. This is distinct from the existing decimal-cast thread: the conversion
mismatch was pre-existing, but changing formerly retained writers to the FE
value creates the new cross-version incompatibility. Please preserve the old
representation for previously legal shapes or provide a compatible
migration/canonicalization strategy, and test a pre-fix serialized state merged
wit
h a new one.
##########
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:
[P2] Preserve this sign when a folded FLOAT literal is translated, not only
when FE widens it first. `FloatLiteral.toLegacyLiteral` still calls inherited
`getDouble()`, which reparses `Float.toString(value)`; Java renders a negative
FLOAT NaN as `NaN`, so the legacy literal becomes positive. A dynamic tree such
as `signbit(CAST(IF(number=0, CAST('-nan(foo)' AS FLOAT), CAST(1 AS FLOAT)) AS
DOUBLE))` therefore loses the sign during translation, whereas
`debug_skip_fold_constant` lets BE parse the retained negative string. This is
distinct from the resolved signed-NaN thread because its case widened and
folded before translation. Please translate the primitive FLOAT value with its
raw sign and add a slot-dependent fold-on/off case.
--
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]
