Use Double.NEGATIVE_INFINITY and Double.POSITIVE_INFINITY #4496
Use Double.NEGATIVE_INFINITY and Double.POSITIVE_INFINITY #4496gianm merged 6 commits intoapache:masterfrom
Conversation
…Double.MIN_VALUE and Double.MAX_VALUE, same for Float
| <property name="message" value="Use Comparators.naturalNullsFirst() instead of Ordering.natural().nullsFirst()"/> | ||
| </module> | ||
|
|
||
| <module name="Regexp"> |
There was a problem hiding this comment.
Do these patterns need to be prohibited? I'm not sure.
There was a problem hiding this comment.
I think yes, because they are 1) never needed in the codebase so far 2) bug-prone, people use them instead of Infinity constants.
|
|
||
| mergedPositions[currentIndex] = mm0; | ||
| //mergedPositions[nextIndex] = Float.MAX_VALUE; // for debugging | ||
| //mergedPositions[nextIndex] = Float.POSITIVE_INFINITY; // for debugging |
There was a problem hiding this comment.
Looks that should be removed.
| heapSize = heapDelete(heap, reverseIndex, heapSize, reverseIndex[currentIndex], deltas); | ||
|
|
||
| //deltas[currentIndex] = Float.MAX_VALUE; // for debugging | ||
| //deltas[currentIndex] = Float.POSITIVE_INFINITY; // for debugging |
There was a problem hiding this comment.
Looks that should be removed.
|
|
||
| // mark the merged bin as invalid | ||
| // deltas[nextIndex] = Float.MAX_VALUE; // for debugging | ||
| // deltas[nextIndex] = Float.POSITIVE_INFINITY; // for debugging |
There was a problem hiding this comment.
Looks that should be removed.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Looks good, but would appreciate more unit tests verifying that the behavior is good now (such as for the doubleMin/doubleMax aggregators). Especially to verify that all the code paths involved do deal well with infinities.
|
@leventov do you have any thoughts on my comment about tests? This is one of the last patches blocking 0.10.1-rc2 so I'm hoping we can resolve it quickly. |
|
Also the json serde needs to make sure it handles the values for positive and negative infinity as well |
|
@gianm I have another patch I want to include into 0.10.1, not published yet |
|
Thanks for adding some tests. I would prefer one for doubleMin/doubleMax too but will not consider that blocking to the PR. Please resolve conflicts too and this looks good to me. |
|
@gianm doubleMin/doubleMax bug couldn't probably exploited now. It needs to have EvalExpr of null converted from float column, I didn't find a way to do that so that Calcite's type checker doesn't complain. |
| <property name="message" value="Use Float.POSITIVE_INFINITY"/> | ||
| </module> | ||
| <module name="Regexp"> | ||
| <property name="format" value="Float\.MIN_VALUE"/> |
There was a problem hiding this comment.
not sure how this check style will work. Does this means any usage of Float.MIN_VALUE is prohibited ?
There was a problem hiding this comment.
yes; see #4496 (comment). I guess if anyone needs it for real then the checkstyle rule could be removed or altered.
* Use Double.NEGATIVE_INFINITY and Double.POSITIVE_INFINITY instead of Double.MIN_VALUE and Double.MAX_VALUE, same for Float * Replace usages in comments * Fix RTree * Remove commented code * Add tests
| float[] initMaxCoords = new float[numDims]; | ||
| Arrays.fill(initMinCoords, -Float.MAX_VALUE); | ||
| Arrays.fill(initMaxCoords, Float.MAX_VALUE); | ||
| Arrays.fill(initMinCoords, Float.NEGATIVE_INFINITY); |
There was a problem hiding this comment.
I am not familiar with the code of Rtree but am not sure if this change really make sense. After this change any operation on the root node coordinate will always return infinity, while before i can have for instance an incremental change over the root coordinate.
Eg, node.getMinCoordinates()[0] [* or /] 100 will return Infinity if we use Float.NEGATIVE_INFINITY as oppose to it will have a defined value if we keep Float.MAX_VALUE
There was a problem hiding this comment.
I think it should be ok. I didn't check the full code around RTree in druid, but usually the min/max coordinates of an RTree node represents a minimum range covering all coordinates of the children nodes. It is used for early pruning when traversing the tree. So, I think the min/max coordinates of the root node shouldn't used for any computation.
|
@gianm was still looking to review this PR... |
| if (ratio >= 1) { | ||
| // handle very unlikely case that value is > 2^64 | ||
| return Double.MAX_VALUE; | ||
| return Double.POSITIVE_INFINITY; |
There was a problem hiding this comment.
Same as comment above why this need to be +infinity and not just Max_value ?
Ah, sorry, didn't realize that. I'd say keep reviewing and if you uncover anything that needs to change let's get it in to 0.10.1. |
5 participants
Instead of Double.MIN_VALUE and Double.MAX_VALUE, same for Float.
Double.MIN_VALUE is the smallest positive non-zero double value, not "very negative" value.
The effect e. g. is that
DoubleMaxAggregatorwith expression column could never return anything negative (seems so).Also there is a buginRTreewhere maxCoords are filled withFloat.MAX_VALUE, and opposite for minCoords.