github-actions[bot] commented on code in PR #68731:
URL: https://github.com/apache/doris/pull/68731#discussion_r4226506758
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/NumericArithmetic.java:
##########
@@ -401,9 +402,8 @@ public static Expression ceil(DecimalV3Literal first,
IntegerLiteral second) {
*/
@ExecFunction(name = "ceil")
public static Expression ceil(DoubleLiteral first) {
- DecimalV3Literal middleResult = DecimalV3Literal.createWithoutCheck256(
- new BigDecimal(Double.toString(first.getValue())));
- return new DoubleLiteral(middleResult.roundCeiling(0).getDouble());
+ // BE uses std::ceil(), which keeps the sign of a zero result.
Review Comment:
[P1] Align FE numeric comparison with the newly preserved negative zero.
`ceil(CAST(-0.5 AS DOUBLE))` now folds to `DoubleLiteral(-0.0)`, but
`NumericLiteral.compareTo` calls `Double.compare(-0.0, +0.0)` and returns -1.
FE therefore folds `ceil(CAST(-0.5 AS DOUBLE)) = CAST(0 AS DOUBLE)` to FALSE,
while BE treats the zeros as equal; `RangeInference` can also erase `x >= +0
AND x <= ceil(-0.5)` even though `x = 0` qualifies. Unary `round(-0.4)` has the
same path. Please correct the shared numeric comparator semantics and cover
folded predicates and ranges.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/NumericArithmetic.java:
##########
@@ -364,9 +364,10 @@ public static Expression round(DecimalV3Literal first,
IntegerLiteral second) {
*/
@ExecFunction(name = "round")
public static Expression round(DoubleLiteral first) {
- DecimalV3Literal middleResult = DecimalV3Literal.createWithoutCheck256(
- new BigDecimal(Double.toString(first.getValue())));
- return new DoubleLiteral(middleResult.round(0).getDouble());
+ // BE uses std::round(): half away from zero, and a zero result keeps
the sign of the
+ // argument. Rounding through BigDecimal at scale 0 collapses -0.4 to
positive zero.
+ double value = first.getValue();
+ return new DoubleLiteral(Math.copySign(Math.floor(Math.abs(value) +
0.5), value));
Review Comment:
[P1] Avoid rounding the input while computing the rounding threshold. For
`0.49999999999999994` (the representable DOUBLE immediately below 0.5),
`Math.abs(value) + 0.5` becomes exactly `1.0`, so FE folds
`round(CAST(0.49999999999999994 AS DOUBLE))` to 1 even though BE `std::round`
returns 0; the negative input becomes -1 instead of -0. The same expression
changes an already integral `4503599627370497.0` to `4503599627370498.0`.
Please use a half-away-from-zero algorithm that does not perform this inexact
addition, and cover both boundaries in folding tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/NumericArithmetic.java:
##########
@@ -401,9 +402,8 @@ public static Expression ceil(DecimalV3Literal first,
IntegerLiteral second) {
*/
@ExecFunction(name = "ceil")
public static Expression ceil(DoubleLiteral first) {
- DecimalV3Literal middleResult = DecimalV3Literal.createWithoutCheck256(
- new BigDecimal(Double.toString(first.getValue())));
- return new DoubleLiteral(middleResult.roundCeiling(0).getDouble());
+ // BE uses std::ceil(), which keeps the sign of a zero result.
Review Comment:
[P1] Use SQL value equality where the newly preserved negative zero enters
sets. `ceil(CAST(-0.5 AS DOUBLE)) IN (CAST(0 AS DOUBLE), CAST(1 AS DOUBLE))`
reaches `visitInPredicate`, whose `Literal.equals` check treats -0.0 and +0.0
as different, so FE folds FALSE; BE `HybridSet` returns TRUE.
`RangeInference.intersect` similarly uses `retainAll` and can erase `x =
ceil(-0.5) AND x = +0` even though `x = 0` qualifies. Correct these semantic
equality sites, while retaining sign-sensitive expression identity for
`signbit`, and add composed `IN` and range tests.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/executable/NumericArithmeticTest.java:
##########
@@ -86,4 +86,34 @@ private void assertInterval(int expected, long compareValue,
long... thresholds)
new BigIntLiteral(compareValue), thresholdLiterals);
Assertions.assertEquals(expected, result.getValue());
}
+
+ @Test
+ public void testCeilAndRoundKeepSignedZero() {
+ DoubleLiteral ceil = (DoubleLiteral) NumericArithmetic.ceil(new
DoubleLiteral(-0.5));
+ Assertions.assertTrue(isNegativeZero(ceil.getValue()));
+
+ DoubleLiteral round = (DoubleLiteral) NumericArithmetic.round(new
DoubleLiteral(-0.4));
Review Comment:
[P2] Cover the explicit scale-zero DOUBLE overloads as well. The new test
and implementation handle only unary calls, but `round(CAST(-0.4 AS DOUBLE),
0)` and `ceil(CAST(-0.5 AS DOUBLE), 0)` still use the BigDecimal overloads
below and FE folds both to positive zero; BE's scale-zero dispatch calls
`std::round`/`std::ceil` and returns negative zero. This leaves `signbit`
dependent on whether the argument is constant or a column for the same public
functions. Please extend the fix and a folded-expression test to these
overloads (or explicitly arrange a tracked follow-up if this PR must remain
unary-only).
--
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]
