Updated patch with the changed parameter validation, updated javadoc and added
test coverage.
# HG changeset patch
# User chris_dennis
# Date 1491945909 14400
# Tue Apr 11 17:25:09 2017 -0400
# Node ID c87cb7f14db71cff2bb4f0a7490b77cfe7ac07b6
# Parent fbedc2de689fd9fe693115630225e2008827c4ec
8178117: Add public state constructors for Int/Long/DoubleSummaryStatistics
diff --git a/src/java.base/share/classes/java/util/DoubleSummaryStatistics.java
b/src/java.base/share/classes/java/util/DoubleSummaryStatistics.java
--- a/src/java.base/share/classes/java/util/DoubleSummaryStatistics.java
+++ b/src/java.base/share/classes/java/util/DoubleSummaryStatistics.java
@@ -76,6 +76,47 @@
public DoubleSummaryStatistics() { }
/**
+ * Construct a non-empty instance with the supplied count, min, max, and
sum.
+ *
+ * <p>If {@code count} is zero then the remaining parameters are ignored
and
+ * an empty instance is constructed.
+ *
+ * <p>If the arguments are inconsistent then an {@code
IllegalArgumentException}
+ * is thrown. The necessary consistent argument conditions are:
+ * <ul>
+ * <li>{@code count} ≥ 0</li>
+ * <li>{@code min} ≤ {@code max}</li>
+ * </ul>
+ * <p>The enforcement of argument correctness means that the retrieved set
+ * of values from a {@code DoubleSummaryStatistics} instance may not be a
legal
+ * set of arguments for this constructor due to arithmetic overflow in the
+ * count field of the original instance. These conditions are not
sufficient
+ * to prevent the creation of an internally inconsistent instance. An
example
+ * of such a state would be an instance with: {@code count} = 2, {@code
min} = 1,
+ * {@code max} = 2, and {@code sum} = 0.
+ *
+ * @param count the count of values
+ * @param min the minimum value
+ * @param max the maximum value
+ * @param sum the sum of all values
+ * @throws IllegalArgumentException if the arguments are inconsistent
+ */
+ public DoubleSummaryStatistics(long count, double min, double max, double
sum) throws IllegalArgumentException {
+ if (count < 0L) {
+ throw new IllegalArgumentException("Negative count value");
+ } else if (count > 0L) {
+ if (min > max) throw new IllegalArgumentException("Minimum greater
than maximum");
+
+ this.count = count;
+ this.sum = sum;
+ this.simpleSum = sum;
+ this.sumCompensation = 0.0d;
+ this.min = min;
+ this.max = max;
+ }
+ }
+
+ /**
* Records another value into the summary information.
*
* @param value the input value
diff --git a/src/java.base/share/classes/java/util/IntSummaryStatistics.java
b/src/java.base/share/classes/java/util/IntSummaryStatistics.java
--- a/src/java.base/share/classes/java/util/IntSummaryStatistics.java
+++ b/src/java.base/share/classes/java/util/IntSummaryStatistics.java
@@ -27,6 +27,9 @@
import java.util.function.IntConsumer;
import java.util.stream.Collector;
+import static java.lang.Math.multiplyExact;
+import static java.lang.Math.multiplyFull;
+
/**
* A state object for collecting statistics such as count, min, max, sum, and
* average.
@@ -76,6 +79,45 @@
public IntSummaryStatistics() { }
/**
+ * Construct a non-empty instance with the supplied count, min, max, and
sum.
+ *
+ * <p>If {@code count} is zero then the remaining parameters are ignored
and
+ * an empty instance is constructed.
+ *
+ * <p>If the arguments are inconsistent then an {@code
IllegalArgumentException}
+ * is thrown. The necessary consistent argument conditions are:
+ * <ul>
+ * <li>{@code count} ≥ 0</li>
+ * <li>{@code min} ≤ {@code max}</li>
+ * </ul>
+ * <p>The enforcement of argument correctness means that the retrieved set
+ * of values from an {@code IntSummaryStatistics} instance may not be a
legal
+ * set of arguments for this constructor due to arithmetic overflow in the
+ * count field of the original instance. These conditions are not
sufficient
+ * to prevent the creation of an internally inconsistent instance. An
example
+ * of such a state would be an instance with: {@code count} = 2, {@code
min} = 1,
+ * {@code max} = 2, and {@code sum} = 0.
+ *
+ * @param count the count of values
+ * @param min the minimum value
+ * @param max the maximum value
+ * @param sum the sum of all values
+ * @throws IllegalArgumentException if the arguments are inconsistent
+ */
+ public IntSummaryStatistics(long count, int min, int max, long sum) throws
IllegalArgumentException {
+ if (count < 0L) {
+ throw new IllegalArgumentException("Negative count value");
+ } else if (count > 0L) {
+ if (min > max) throw new IllegalArgumentException("Minimum greater
than maximum");
+
+ this.count = count;
+ this.sum = sum;
+ this.min = min;
+ this.max = max;
+ }
+ }
+
+ /**
* Records a new value into the summary information
*
* @param value the input value
diff --git a/src/java.base/share/classes/java/util/LongSummaryStatistics.java
b/src/java.base/share/classes/java/util/LongSummaryStatistics.java
--- a/src/java.base/share/classes/java/util/LongSummaryStatistics.java
+++ b/src/java.base/share/classes/java/util/LongSummaryStatistics.java
@@ -28,6 +28,8 @@
import java.util.function.LongConsumer;
import java.util.stream.Collector;
+import static java.lang.Math.multiplyExact;
+
/**
* A state object for collecting statistics such as count, min, max, sum, and
* average.
@@ -77,6 +79,45 @@
public LongSummaryStatistics() { }
/**
+ * Construct a non-empty instance with the supplied count, min, max, and
sum.
+ *
+ * <p>If {@code count} is zero then the remaining parameters are ignored
and
+ * an empty instance is constructed.
+ *
+ * <p>If the arguments are inconsistent then an {@code
IllegalArgumentException}
+ * is thrown. The necessary consistent argument conditions are:
+ * <ul>
+ * <li>{@code count} ≥ 0</li>
+ * <li>{@code min} ≤ {@code max}</li>
+ * </ul>
+ * <p>The enforcement of argument correctness means that the retrieved set
+ * of values from a {@code LongSummaryStatistics} instance may not be a
legal
+ * set of arguments for this constructor due to arithmetic overflow in the
+ * count field of the original instance. These conditions are not
sufficient
+ * to prevent the creation of an internally inconsistent instance. An
example
+ * of such a state would be an instance with: {@code count} = 2, {@code
min} = 1,
+ * {@code max} = 2, and {@code sum} = 0.
+ *
+ * @param count the count of values
+ * @param min the minimum value
+ * @param max the maximum value
+ * @param sum the sum of all values
+ * @throws IllegalArgumentException if the arguments are inconsistent
+ */
+ public LongSummaryStatistics(long count, long min, long max, long sum)
throws IllegalArgumentException {
+ if (count < 0L) {
+ throw new IllegalArgumentException("Negative count value");
+ } else if (count > 0L) {
+ if (min > max) throw new IllegalArgumentException("Minimum greater
than maximum");
+
+ this.count = count;
+ this.sum = sum;
+ this.min = min;
+ this.max = max;
+ }
+ }
+
+ /**
* Records a new {@code int} value into the summary information.
*
* @param value the input value
diff --git
a/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectAndSummaryStatisticsTest.java
b/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectAndSummaryStatisticsTest.java
---
a/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectAndSummaryStatisticsTest.java
+++
b/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectAndSummaryStatisticsTest.java
@@ -91,11 +91,19 @@
instances.add(countTo(1000).stream().mapToInt(i ->
i).collect(IntSummaryStatistics::new,
IntSummaryStatistics::accept,
IntSummaryStatistics::combine));
+ instances.add(countTo(1000).stream().mapToInt(i -> i).collect(() ->
new IntSummaryStatistics(0, -1, 1001, 2),
+
IntSummaryStatistics::accept,
+
IntSummaryStatistics::combine));
instances.add(countTo(1000).parallelStream().collect(Collectors.summarizingInt(i
-> i)));
instances.add(countTo(1000).parallelStream().mapToInt(i ->
i).summaryStatistics());
instances.add(countTo(1000).parallelStream().mapToInt(i ->
i).collect(IntSummaryStatistics::new,
IntSummaryStatistics::accept,
IntSummaryStatistics::combine));
+ instances.add(countTo(1000).parallelStream().mapToInt(i ->
i).collect(() -> new IntSummaryStatistics(0, -1, 1001, 2),
+
IntSummaryStatistics::accept,
+
IntSummaryStatistics::combine));
+ IntSummaryStatistics original = instances.get(0);
+ instances.add(new IntSummaryStatistics(original.getCount(),
original.getMin(), original.getMax(), original.getSum()));
for (IntSummaryStatistics stats : instances) {
assertEquals(stats.getCount(), 1000);
@@ -104,6 +112,9 @@
assertEquals(stats.getMax(), 1000);
assertEquals(stats.getMin(), 1);
}
+
+ expectThrows(IllegalArgumentException.class, () -> new
IntSummaryStatistics(-1, 0, 0, 0));
+ expectThrows(IllegalArgumentException.class, () -> new
IntSummaryStatistics(1, 3, 2, 0));
}
@@ -114,11 +125,20 @@
instances.add(countTo(1000).stream().mapToLong(i ->
i).collect(LongSummaryStatistics::new,
LongSummaryStatistics::accept,
LongSummaryStatistics::combine));
+ instances.add(countTo(1000).stream().mapToLong(i -> i).collect(() ->
new LongSummaryStatistics(0, -1, 1001, 2),
+
LongSummaryStatistics::accept,
+
LongSummaryStatistics::combine));
instances.add(countTo(1000).parallelStream().collect(Collectors.summarizingLong(i
-> i)));
instances.add(countTo(1000).parallelStream().mapToLong(i ->
i).summaryStatistics());
instances.add(countTo(1000).parallelStream().mapToLong(i ->
i).collect(LongSummaryStatistics::new,
LongSummaryStatistics::accept,
LongSummaryStatistics::combine));
+ instances.add(countTo(1000).parallelStream().mapToLong(i ->
i).collect(() -> new LongSummaryStatistics(0, -1, 1001, 2),
+
LongSummaryStatistics::accept,
+
LongSummaryStatistics::combine));
+
+ LongSummaryStatistics original = instances.get(0);
+ instances.add(new LongSummaryStatistics(original.getCount(),
original.getMin(), original.getMax(), original.getSum()));
for (LongSummaryStatistics stats : instances) {
assertEquals(stats.getCount(), 1000);
@@ -127,6 +147,9 @@
assertEquals(stats.getMax(), 1000L);
assertEquals(stats.getMin(), 1L);
}
+
+ expectThrows(IllegalArgumentException.class, () -> new
LongSummaryStatistics(-1, 0, 0, 0));
+ expectThrows(IllegalArgumentException.class, () -> new
LongSummaryStatistics(1, 3, 2, 0));
}
public void testDoubleStatistics() {
@@ -136,11 +159,20 @@
instances.add(countTo(1000).stream().mapToDouble(i ->
i).collect(DoubleSummaryStatistics::new,
DoubleSummaryStatistics::accept,
DoubleSummaryStatistics::combine));
+ instances.add(countTo(1000).stream().mapToDouble(i -> i).collect(() ->
new DoubleSummaryStatistics(0, -1, 1001, 2),
+
DoubleSummaryStatistics::accept,
+
DoubleSummaryStatistics::combine));
instances.add(countTo(1000).parallelStream().collect(Collectors.summarizingDouble(i
-> i)));
instances.add(countTo(1000).parallelStream().mapToDouble(i ->
i).summaryStatistics());
instances.add(countTo(1000).parallelStream().mapToDouble(i ->
i).collect(DoubleSummaryStatistics::new,
DoubleSummaryStatistics::accept,
DoubleSummaryStatistics::combine));
+ instances.add(countTo(1000).parallelStream().mapToDouble(i ->
i).collect(() -> new DoubleSummaryStatistics(0, -1, 1001, 2),
+
DoubleSummaryStatistics::accept,
+
DoubleSummaryStatistics::combine));
+
+ DoubleSummaryStatistics original = instances.get(0);
+ instances.add(new DoubleSummaryStatistics(original.getCount(),
original.getMin(), original.getMax(), original.getSum()));
for (DoubleSummaryStatistics stats : instances) {
assertEquals(stats.getCount(), 1000);
@@ -149,5 +181,8 @@
assertEquals(stats.getMax(), 1000.0);
assertEquals(stats.getMin(), 1.0);
}
+
+ expectThrows(IllegalArgumentException.class, () -> new
DoubleSummaryStatistics(-1, 0, 0, 0));
+ expectThrows(IllegalArgumentException.class, () -> new
DoubleSummaryStatistics(1, 3, 2, 0));
}
}
> On Apr 11, 2017, at 4:48 PM, Chris Dennis <[email protected]> wrote:
>
> Color me confused… what would the javadoc on the parameter say? It could I
> guess have an @implNote documenting the meanings of the parameters… but then
> what use is it? The proposed API simply limits the precision with which a
> DoubleSummaryStatistic can be copied to be the same as the precision with
> which it can be “accessed”. That doesn’t seem an enormous problem since I
> suspect that bulk of usages would be to marshall a “finished” instance and
> therefore there is no real loss occuring. If we wanted to support arbitrary
> precision wouldn’t it be better to have a constructor variant that took a
> BigDecimal, and a matching getPreciseSum() that returned a BigDecimal?
>
> Chris
>
>> On Apr 11, 2017, at 4:16 PM, joe darcy <[email protected]> wrote:
>>
>> On an API design note, I would prefer to DoubleSummaryStatistics took a
>> double... argument to pass in the state of the summation. This flexibility
>> is necessary to more fully preserve the computed sum. Also, the we want
>> flexibility to change the amount of internal state DoubleSummaryStats keeps
>> so we don't want to hard-code that into as aspect of the API.
>>
>> Thanks,
>>
>> -Joe
>>
>>
>> On 4/11/2017 12:53 PM, Paul Sandoz wrote:
>>> Hi Chris,
>>>
>>> Thanks for looking at this.
>>>
>>> There is some rudimentary testing using streams in
>>> CollectAndSummaryStatisticsTest.java.
>>>
>>> I think we should enforce constraints where we reliably can:
>>>
>>> 1) count >= 0
>>>
>>> 2) count = 0, then min/max/sum are ignored and it’s as if the default
>>> constructor was called. (I thought it might be appropriate to reject since
>>> a caller might be passing in incorrect information in error, but the
>>> defaults for min/max are important and would be a burden for the caller to
>>> pass those values.) In this respect having count as the first parameter of
>>> the constructor would be useful.
>>>
>>> 3) min <= max
>>>
>>> Since for count > 0 the constraints, count * max >= sum, count * min <=
>>> sum, cannot be reliably enforced due to overflow i am inclined to not
>>> bother and just document.
>>>
>>>
>>> Note this is gonna be blocked from pushing until the new Compatibility and
>>> Specification Review (CSR) process is opened up, which i understand is
>>> “soon"ish :-) but that should not block adding some further tests in the
>>> interim and tidying up the javadoc.
>>>
>>> Paul.
>>>
>>>
>>>> On 6 Apr 2017, at 07:08, Chris Dennis <[email protected]> wrote:
>>>>
>>>> Hi Paul (et al)
>>>>
>>>> Like all things API there are wrinkles here when it comes to implementing.
>>>>
>>>> This patch isn’t final, there appears to be no existing test coverage for
>>>> these classes beyond testing the compensating summation used in the double
>>>> implementation, and I left off adding any until it was decided how much
>>>> parameter validation we want (since that’s the only testing that can
>>>> really occur here).
>>>>
>>>> The wrinkles relate to the issues around instances that have suffered
>>>> overflow in count and/or sum which the existing implementation doesn’t
>>>> defend against:
>>>>
>>>> * I chose to ignore all parameters if 'count == 0’ and just leave the
>>>> instance empty. I held off from validating min, max and count however.
>>>> This obviously 'breaks things’ (beyond how broken they would already be)
>>>> if count == 0 only due to overflow.
>>>> * I chose to fail if count is negative.
>>>> * I chose to enforce that max and min are logically consistent with count
>>>> and sum, this can break the moment either one of the overflows as well
>>>> (I’m also pretty sure that the FPU logic is going to suffer here too -
>>>> there might be some magic I can do with a pessimistic bit of rounding on
>>>> the FPU multiplication result).
>>>>
>>>> I intentionally went with the most aggressive parameter validation here to
>>>> establish one end of what could be implemented. There are arguments for
>>>> this and also arguments for the other extreme (no validation at all).
>>>> Personally I’m in favor of the current version where the creation will
>>>> fail if the inputs are only possible through overflow (modulo fixing the
>>>> FPU precision issues above) even though it’s at odds with approach of the
>>>> existing implementation.
>>>>
>>>> Would appreciate thoughts/feedback. Thanks.
>>>>
>>>> Chris
>>>>
>>>>
>>>> P.S. I presume tests would be required for the parameter checking (once it
>>>> is finalized)?
>>>>
>>>> <8178117.patch>
>>
>
