[GitHub] [druid] maytasm commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values

[GitBox <gi...@apache.org>]

maytasm commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r439132322



##########
File path: docs/misc/math-expr.md
##########
@@ -141,7 +141,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |pow|pow(x, y) would return the value of the x raised to the power of y|
 |remainder|remainder(x, y) would return the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) would return value that is closest in value to x and is equal to a mathematical integer|
-|round|round(x, y) would return the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points.|
+|round|round(x, y) would return the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is either `NaN` or infinity, this will return 0. |

Review comment:
       explicitly mention positive and negative infinity

##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -737,7 +745,12 @@ private ExprEval eval(ExprEval param, int scale)
       if (param.type() == ExprType.LONG) {
         return ExprEval.of(BigDecimal.valueOf(param.asLong()).setScale(scale, RoundingMode.HALF_UP).longValue());
       } else if (param.type() == ExprType.DOUBLE) {
-        return ExprEval.of(BigDecimal.valueOf(param.asDouble()).setScale(scale, RoundingMode.HALF_UP).doubleValue());
+        double val = param.asDouble();
+        if (Double.isNaN(val) || val == Double.POSITIVE_INFINITY || val == Double.NEGATIVE_INFINITY) {
+          // This is the behavior of Math.round()
+          return ExprEval.of(0L);

Review comment:
       Should we use Math.round(val) here instead of hard-code to 0?

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java
##########
@@ -122,6 +122,7 @@ public void reduce(
             if (exprResult.type() == ExprType.LONG) {
               bigDecimal = BigDecimal.valueOf(exprResult.asLong());
             } else {
+              // if exprResult evaluates to Nan or infinity, this will throw a NumberFormatException

Review comment:
       Wondering what is your thought on making this reduce to value of 0 (same as the other stuff)

##########
File path: docs/querying/sql.md
##########
@@ -287,7 +287,7 @@ to FLOAT. At runtime, Druid will widen 32-bit floats to 64-bit for most expressi
 |`SQRT(expr)`|Square root.|
 |`TRUNCATE(expr[, digits])`|Truncate expr to a specific number of decimal digits. If digits is negative, then this truncates that many places to the left of the decimal point. Digits defaults to zero if not specified.|
 |`TRUNC(expr[, digits])`|Synonym for `TRUNCATE`.|
-|`ROUND(expr[, digits])`|`ROUND(x, y)` would return the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points.|
+|`ROUND(expr[, digits])`|`ROUND(x, y)` would return the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If `expr` evaluates to either `NaN` or infinity, this will return 0. |

Review comment:
       explicitly mention positive and negative infinity




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org