You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/10 08:33:37 UTC
[GitHub] [druid] suneet-s opened a new pull request #10014: ROUND and having comparators correctly handle doubles
suneet-s opened a new pull request #10014:
URL: https://github.com/apache/druid/pull/10014
### Description
Double.NaN, Double.POSITIVE_INFINITY and Double.NEGATIVE_INFINITY are not real
numbers. Because of this, they can not be converted to BigDecimal and instead
throw a NumberFormatException.
This change adds support for calculations that produce these numbers either
for use in the `ROUND` function or the HavingSpecMetricComparator by not
attempting to convert the number to a BigDecimal.
The bug in `ROUND` was first introduced in #7224 where we added the ability to
round to any decimal place. This PR changes the behavior back to using
`Math.round` if we recognize a number that can not be converted to a
BigDecimal.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
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
[GitHub] [druid] maytasm commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by 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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440901322
##########
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:
I don't think so. I was afraid documenting all the edge cases would make the docs too verbose.
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r441156987
##########
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`, this will return 0. |
Review comment:
```suggestion
|`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`, `expr` will be converted to 0. If `expr` is infinity, `expr` will be converted to the nearest finite double. |
```
##########
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 `NaN`, this will return 0. |
Review comment:
```suggestion
|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 `NaN`, x will return 0. If x is infinity, x will be converted to the nearest finite double. |
```
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440505144
##########
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:
I thought this might produce confusing behavior if the literal was used in a comparison operation. For eg. `1 < POSITIVE_INFINITY` would return false because `POSITIVE_INFINITY` would evaluate to if we changed the code here.
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440505620
##########
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:
I guess the same argument can be made if we compared 2 columns `x < y` and x was `1`, y was `POSITIVE_INFINITY` then the result of the comparison would be false after this change.
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s merged pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #10014:
URL: https://github.com/apache/druid/pull/10014
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440316096
##########
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:
I wasn't sure of the perf implications of calling `Math.round` It looks like it calls out to some native function, does some bit magic and then finally just casts the object to a long - which is how we get `0L`
So I thought it would be better to just return 0 directly
----------------------------------------------------------------
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
[GitHub] [druid] maytasm commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440534065
##########
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:
Do you know if this is mention in Druid docs?
----------------------------------------------------------------
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
[GitHub] [druid] maytasm commented on a change in pull request #10014: ROUND and having comparators correctly handle special double values
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r440534184
##########
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:
Sounds good to me.
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10014: ROUND and having comparators correctly handle doubles
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10014:
URL: https://github.com/apache/druid/pull/10014#discussion_r437956928
##########
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:
I wasn't sure how to deal with non numbers here, so I decided to leave the behavior as is and let it fail. It's unclear to me if Calcite will allow us to get here. In my local testing, I've seen NumberFormatExceptions thrown in Calcite when I tried to write sql expressions that would compute to Nan or infinity (like `0D / 0`).
My thoughts were this is an edge case so it's ok to leave this behavior as is.
----------------------------------------------------------------
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