From d168a4271e8ca7b9d3a15aafc9dbeeb66dc3286e Mon Sep 17 00:00:00 2001 From: Roman Leventov Date: Fri, 7 Jul 2017 10:10:13 -0500 Subject: [PATCH] Use Double.NEGATIVE_INFINITY and Double.POSITIVE_INFINITY (#4496) * 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 --- .../JacksonExtremeDoubleValuesSerdeTest.java | 41 +++++++++++++++++++ .../io/druid/collections/spatial/Node.java | 4 +- .../io/druid/collections/spatial/RTree.java | 6 +-- .../split/LinearGutmanSplitStrategy.java | 8 ++-- .../split/QuadraticGutmanSplitStrategy.java | 4 +- codestyle/checkstyle.xml | 21 ++++++++++ .../histogram/ApproximateHistogram.java | 20 +++------ .../io/druid/hll/HyperLogLogCollector.java | 2 +- .../druid/hll/HyperLogLogCollectorTest.java | 2 +- .../DoubleMaxAggregatorFactory.java | 8 +++- .../DoubleMinAggregatorFactory.java | 8 +++- .../io/druid/query/aggregation/Histogram.java | 4 +- .../HistogramBufferAggregator.java | 4 +- .../druid/segment/FloatDimensionIndexer.java | 4 +- .../query/aggregation/HistogramTest.java | 19 +++++++++ .../data/CompressedFloatsSerdeTest.java | 4 +- .../incremental/IncrementalIndexTest.java | 4 +- 17 files changed, 123 insertions(+), 40 deletions(-) create mode 100644 api/src/test/java/io/druid/jackson/JacksonExtremeDoubleValuesSerdeTest.java diff --git a/api/src/test/java/io/druid/jackson/JacksonExtremeDoubleValuesSerdeTest.java b/api/src/test/java/io/druid/jackson/JacksonExtremeDoubleValuesSerdeTest.java new file mode 100644 index 00000000000..789fd82def9 --- /dev/null +++ b/api/src/test/java/io/druid/jackson/JacksonExtremeDoubleValuesSerdeTest.java @@ -0,0 +1,41 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.jackson; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; + +public class JacksonExtremeDoubleValuesSerdeTest +{ + @Test + public void testExtremeDoubleValuesSerde() throws IOException + { + ObjectMapper objectMapper = new ObjectMapper(); + for (double value : new double[] {Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY}) { + String serialized = objectMapper.writeValueAsString(value); + Assert.assertEquals(new Double(value), objectMapper.readValue(serialized, Double.class)); + } + String negativeInfinityString = objectMapper.writeValueAsString(Double.NaN); + Assert.assertTrue(objectMapper.readValue(negativeInfinityString, Double.class).isNaN()); + } +} diff --git a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/Node.java b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/Node.java index 1206083799c..e93df949c43 100755 --- a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/Node.java +++ b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/Node.java @@ -151,9 +151,9 @@ public class Node { boolean retVal = false; float[] minCoords = new float[getNumDims()]; - Arrays.fill(minCoords, Float.MAX_VALUE); + Arrays.fill(minCoords, Float.POSITIVE_INFINITY); float[] maxCoords = new float[getNumDims()]; - Arrays.fill(maxCoords, -Float.MAX_VALUE); + Arrays.fill(maxCoords, Float.NEGATIVE_INFINITY); for (Node child : getChildren()) { for (int i = 0; i < getNumDims(); i++) { diff --git a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/RTree.java b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/RTree.java index 74ad9426802..6cce6b90e75 100755 --- a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/RTree.java +++ b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/RTree.java @@ -128,8 +128,8 @@ public class RTree { float[] initMinCoords = new float[numDims]; float[] initMaxCoords = new float[numDims]; - Arrays.fill(initMinCoords, -Float.MAX_VALUE); - Arrays.fill(initMaxCoords, Float.MAX_VALUE); + Arrays.fill(initMinCoords, Float.NEGATIVE_INFINITY); + Arrays.fill(initMaxCoords, Float.POSITIVE_INFINITY); return new Node(initMinCoords, initMaxCoords, isLeaf, bitmapFactory); } @@ -178,7 +178,7 @@ public class RTree return node; } - double minCost = Double.MAX_VALUE; + double minCost = Double.POSITIVE_INFINITY; Node optimal = node.getChildren().get(0); for (Node child : node.getChildren()) { double cost = RTreeUtils.getExpansionCost(child, point); diff --git a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/LinearGutmanSplitStrategy.java b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/LinearGutmanSplitStrategy.java index a193f466cff..e4249f35f39 100755 --- a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/LinearGutmanSplitStrategy.java +++ b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/LinearGutmanSplitStrategy.java @@ -58,10 +58,10 @@ public class LinearGutmanSplitStrategy extends GutmanSplitStrategy double bestNormalized = 0.0; for (int i = 0; i < numDims; i++) { - float minCoord = Float.MAX_VALUE; - float maxCoord = -Float.MAX_VALUE; - float highestLowSide = -Float.MAX_VALUE; - float lowestHighside = Float.MAX_VALUE; + float minCoord = Float.POSITIVE_INFINITY; + float maxCoord = Float.NEGATIVE_INFINITY; + float lowestHighside = Float.POSITIVE_INFINITY; + float highestLowSide = Float.NEGATIVE_INFINITY; int highestLowSideIndex = 0; int lowestHighSideIndex = 0; diff --git a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/QuadraticGutmanSplitStrategy.java b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/QuadraticGutmanSplitStrategy.java index 444fc5ecaeb..a1808919d13 100755 --- a/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/QuadraticGutmanSplitStrategy.java +++ b/bytebuffer-collections/src/main/java/io/druid/collections/spatial/split/QuadraticGutmanSplitStrategy.java @@ -37,7 +37,7 @@ public class QuadraticGutmanSplitStrategy extends GutmanSplitStrategy @Override public Node[] pickSeeds(List nodes) { - double highestCost = Double.MIN_VALUE; + double highestCost = Double.NEGATIVE_INFINITY; int[] highestCostIndices = new int[2]; for (int i = 0; i < nodes.size() - 1; i++) { @@ -58,7 +58,7 @@ public class QuadraticGutmanSplitStrategy extends GutmanSplitStrategy @Override public Node pickNext(List nodes, Node[] groups) { - double highestCost = Double.MIN_VALUE; + double highestCost = Double.NEGATIVE_INFINITY; Node costlyNode = null; int counter = 0; int index = -1; diff --git a/codestyle/checkstyle.xml b/codestyle/checkstyle.xml index fc79f49e908..0c994b818c0 100644 --- a/codestyle/checkstyle.xml +++ b/codestyle/checkstyle.xml @@ -71,10 +71,31 @@ + + + + + + + + + + + + + + + + + + + + + diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogram.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogram.java index 50d941f0af2..da4b0851375 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogram.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogram.java @@ -335,11 +335,11 @@ public class ApproximateHistogram // or merge existing bins before inserting the new one int minPos = minDeltaIndex(); - float minDelta = minPos >= 0 ? positions[minPos + 1] - positions[minPos] : Float.MAX_VALUE; + float minDelta = minPos >= 0 ? positions[minPos + 1] - positions[minPos] : Float.POSITIVE_INFINITY; // determine the distance of new value to the nearest bins - final float deltaRight = insertAt < binCount ? positions[insertAt] - value : Float.MAX_VALUE; - final float deltaLeft = insertAt > 0 ? value - positions[insertAt - 1] : Float.MAX_VALUE; + final float deltaRight = insertAt < binCount ? positions[insertAt] - value : Float.POSITIVE_INFINITY; + final float deltaLeft = insertAt > 0 ? value - positions[insertAt - 1] : Float.POSITIVE_INFINITY; boolean mergeValue = false; if (deltaRight < minDelta) { @@ -368,7 +368,7 @@ public class ApproximateHistogram protected int minDeltaIndex() { // determine minimum distance between existing bins - float minDelta = Float.MAX_VALUE; + float minDelta = Float.POSITIVE_INFINITY; int minPos = -1; for (int i = 0; i < binCount - 1; ++i) { float delta = (positions[i + 1] - positions[i]); @@ -886,9 +886,6 @@ public class ApproximateHistogram while (i < numMerge) { // find the smallest delta within the range used for bins - // pick minimum delta by scanning array - //int currentIndex = minIndex(deltas, lastValidIndex); - // pick minimum delta index using min-heap int currentIndex = heap[0]; @@ -908,17 +905,13 @@ public class ApproximateHistogram final float mm0 = (m0 - m1) * w + m1; mergedPositions[currentIndex] = mm0; - //mergedPositions[nextIndex] = Float.MAX_VALUE; // for debugging mergedBins[currentIndex] = sum | APPROX_FLAG_BIT; - //mergedBins[nextIndex] = -1; // for debugging // update deltas and min-heap if (nextIndex == lastValidIndex) { // merged bin is the last => remove the current bin delta from the heap heapSize = heapDelete(heap, reverseIndex, heapSize, reverseIndex[currentIndex], deltas); - - //deltas[currentIndex] = Float.MAX_VALUE; // for debugging } else { // merged bin is not the last => remove the merged bin delta from the heap heapSize = heapDelete(heap, reverseIndex, heapSize, reverseIndex[nextIndex], deltas); @@ -938,9 +931,6 @@ public class ApproximateHistogram siftDown(heap, reverseIndex, reverseIndex[prevIndex], heapSize - 1, deltas); } - // mark the merged bin as invalid - // deltas[nextIndex] = Float.MAX_VALUE; // for debugging - // update last valid index if we merged the last bin if (nextIndex == lastValidIndex) { lastValidIndex = currentIndex; @@ -1037,7 +1027,7 @@ public class ApproximateHistogram private static int minIndex(float[] deltas, int lastValidIndex) { int minIndex = -1; - float min = Float.MAX_VALUE; + float min = Float.POSITIVE_INFINITY; for (int k = 0; k < lastValidIndex; ++k) { float value = deltas[k]; if (value < min) { diff --git a/hll/src/main/java/io/druid/hll/HyperLogLogCollector.java b/hll/src/main/java/io/druid/hll/HyperLogLogCollector.java index f5465bb53e1..e09f35d602e 100644 --- a/hll/src/main/java/io/druid/hll/HyperLogLogCollector.java +++ b/hll/src/main/java/io/druid/hll/HyperLogLogCollector.java @@ -146,7 +146,7 @@ public abstract class HyperLogLogCollector implements Comparable= 1) { // handle very unlikely case that value is > 2^64 - return Double.MAX_VALUE; + return Double.POSITIVE_INFINITY; } else { return -TWO_TO_THE_SIXTY_FOUR * Math.log(1 - ratio); } diff --git a/hll/src/test/java/io/druid/hll/HyperLogLogCollectorTest.java b/hll/src/test/java/io/druid/hll/HyperLogLogCollectorTest.java index 3ef1c525e2f..4eea16f1f68 100644 --- a/hll/src/test/java/io/druid/hll/HyperLogLogCollectorTest.java +++ b/hll/src/test/java/io/druid/hll/HyperLogLogCollectorTest.java @@ -698,7 +698,7 @@ public class HyperLogLogCollectorTest fillBuckets(collector, (byte) 0, (byte) 63); collector.add(new byte[]{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}); - Assert.assertEquals(Double.MAX_VALUE, collector.estimateCardinality(), 1000); + Assert.assertEquals(Double.POSITIVE_INFINITY, collector.estimateCardinality(), 1000); } @Test diff --git a/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java index db928b4b686..a6dc784b911 100644 --- a/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java @@ -86,7 +86,13 @@ public class DoubleMaxAggregatorFactory extends AggregatorFactory private FloatColumnSelector getFloatColumnSelector(ColumnSelectorFactory metricFactory) { - return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MIN_VALUE); + return AggregatorUtil.getFloatColumnSelector( + metricFactory, + macroTable, + fieldName, + expression, + Float.NEGATIVE_INFINITY + ); } @Override diff --git a/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java index 1ef0e12cd84..cd596304608 100644 --- a/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java @@ -87,7 +87,13 @@ public class DoubleMinAggregatorFactory extends AggregatorFactory private FloatColumnSelector getFloatColumnSelector(ColumnSelectorFactory metricFactory) { - return AggregatorUtil.getFloatColumnSelector(metricFactory, macroTable, fieldName, expression, Float.MAX_VALUE); + return AggregatorUtil.getFloatColumnSelector( + metricFactory, + macroTable, + fieldName, + expression, + Float.POSITIVE_INFINITY + ); } @Override diff --git a/processing/src/main/java/io/druid/query/aggregation/Histogram.java b/processing/src/main/java/io/druid/query/aggregation/Histogram.java index b77d2607f70..3beda4ea7ab 100644 --- a/processing/src/main/java/io/druid/query/aggregation/Histogram.java +++ b/processing/src/main/java/io/druid/query/aggregation/Histogram.java @@ -42,8 +42,8 @@ public class Histogram this.breaks = breaks; this.bins = new long[this.breaks.length + 1]; this.count = 0; - this.min = Float.MAX_VALUE; - this.max = Float.MIN_VALUE; + this.min = Float.POSITIVE_INFINITY; + this.max = Float.NEGATIVE_INFINITY; } public Histogram(float[] breaks, long[] bins, float min, float max) { diff --git a/processing/src/main/java/io/druid/query/aggregation/HistogramBufferAggregator.java b/processing/src/main/java/io/druid/query/aggregation/HistogramBufferAggregator.java index 4a6dd9a8508..66b2a2fbd0e 100644 --- a/processing/src/main/java/io/druid/query/aggregation/HistogramBufferAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/HistogramBufferAggregator.java @@ -50,8 +50,8 @@ public class HistogramBufferAggregator implements BufferAggregator final long[] bins = new long[breaks.length + 1]; mutationBuffer.asLongBuffer().put(bins); - mutationBuffer.putFloat(position + minOffset, Float.MAX_VALUE); - mutationBuffer.putFloat(position + maxOffset, Float.MIN_VALUE); + mutationBuffer.putFloat(position + minOffset, Float.POSITIVE_INFINITY); + mutationBuffer.putFloat(position + maxOffset, Float.NEGATIVE_INFINITY); } @Override diff --git a/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java b/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java index a7f5f75450c..763f4edd980 100644 --- a/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/FloatDimensionIndexer.java @@ -69,13 +69,13 @@ public class FloatDimensionIndexer implements DimensionIndexerof( "string", Arrays.asList("A", null, ""), - "float", Arrays.asList(Float.MAX_VALUE, null, ""), + "float", Arrays.asList(Float.POSITIVE_INFINITY, null, ""), "long", Arrays.asList(Long.MIN_VALUE, null, "") ) ) @@ -223,7 +223,7 @@ public class IncrementalIndexTest Row row = index.iterator().next(); Assert.assertEquals(Arrays.asList(new String[]{"", "", "A"}), row.getRaw("string")); - Assert.assertEquals(Arrays.asList(new String[]{"", "", String.valueOf(Float.MAX_VALUE)}), row.getRaw("float")); + Assert.assertEquals(Arrays.asList(new String[]{"", "", String.valueOf(Float.POSITIVE_INFINITY)}), row.getRaw("float")); Assert.assertEquals(Arrays.asList(new String[]{"", "", String.valueOf(Long.MIN_VALUE)}), row.getRaw("long")); }