abhinav-phi commented on code in PR #68731:
URL: https://github.com/apache/doris/pull/68731#discussion_r4296945902
##########
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:
fixed in 8da19ea9. the threshold no longer comes from adding a half to the
input. `round(DoubleLiteral)` now takes the fractional part and compares that:
double magnitude = Math.abs(value);
double truncated = Math.floor(magnitude);
if (magnitude - truncated >= 0.5) {
truncated += 1.0;
}
return new DoubleLiteral(Math.copySign(truncated, value));
`magnitude - truncated` is exact for every finite magnitude of 1 or above,
and below 1 it is just the magnitude itself, so neither of the two cases you
raised can reach a wrong branch. `round(CAST(0.49999999999999994 AS DOUBLE))`
folds to 0.0 and `round(CAST(4503599627370497.0 AS DOUBLE))` comes back
unchanged. both are asserted in NumericArithmeticTest now, along with the sign
of a zero result.
##########
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:
fixed in 8da19ea9. both scale zero DOUBLE overloads now hand off to the
unary binary path instead of going through BigDecimal:
public static Expression round(DoubleLiteral first, IntegerLiteral
second) {
if (second.getValue() == 0) {
return round(first);
}
`ceil(DoubleLiteral, IntegerLiteral)` does the same against `ceil(first)`,
which matches the dispatch the BE makes for scale 0. `round(CAST(-0.4 AS
DOUBLE), 0)` and `ceil(CAST(-0.5 AS DOUBLE), 0)` are both in the parameterized
set now with the sign of the zero result asserted, so the two overloads can not
drift back apart.
##########
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:
fixed in 8da19ea9, in `NumericLiteral.compareTo` rather than at the call
site you anchored on:
double self = this.getDouble();
double otherValue = ((Literal) other).getDouble();
if (self == otherValue) {
return 0;
}
return Double.compare(self, otherValue);
the `==` settles the two signed zeros as one SQL value before
`Double.compare` can order -0.0 below +0.0. every other ordering is untouched,
because for all non zero doubles `==` and `Double.compare` agree, and the NaN
case still falls through to `Double.compare` so the total order over NaN is
preserved. `ceil(CAST(-0.5 AS DOUBLE)) = CAST(0 AS DOUBLE)` now folds to TRUE.
##########
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:
fixed in 8da19ea9, but on DoubleLiteral rather than on `Literal.equals`.
changing `Literal.equals` would move the equality contract for every literal
type at once, so I overrode it where the value is actually a double:
public boolean equals(Object o) {
if (!(o instanceof DoubleLiteral)) {
return false;
}
double other = ((DoubleLiteral) o).getValue();
return value == other || (Double.isNaN(value) &&
Double.isNaN(other));
}
with a matching `computeHashCode` over `Double.hashCode(value == 0.0 ? 0.0 :
value)`, so a folded -0.0 and a +0.0 also land in the same bucket. java
dispatches to the override, so `visitInPredicate` and the set dedup both see
one value, which is the BE behaviour. `ceil(CAST(-0.5 AS DOUBLE)) IN (CAST(0 AS
DOUBLE), CAST(1 AS DOUBLE))` folds to TRUE and is asserted.
one thing i want to flag rather than bury: FloatLiteral has the identical
bit comparison today. i did not change it here because that widening is the
subject of sibling pr #68730 and i did not want two branches editing the same
equality question. if you would prefer this rule on `Literal` so both float and
double get it in one place, say so and i will move it.
--
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]
