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 <chris.w.den...@gmail.com> 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 <joe.da...@oracle.com> 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 <chris.w.den...@gmail.com> 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> >> >