On Feb 11, 2015, at 12:02 AM, Stuart Marks <stuart.ma...@oracle.com> wrote:
> Hi Paul, > > On 2/3/15 5:48 AM, Paul Sandoz wrote: >> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping/webrev/ >> >> This patch adds a new flat mapping collector to Collectors. This can be >> useful if one needs to map 0 or more items into a downstream collector. > > Mostly pretty good, just a couple minor nits. > > Re the comment at TabulatorsTest.java line 513, this isn't a two-level > groupBy anymore, it's a groupByWithMapping (as the test name indicates). > I removed the comment. I also added the bug id to the test. Updated in place: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/ > Given the new tests of closing in FlatMapOpTest.java, is it necessary to have > testFlatMappingClose() in TabulatorsTest.java? > Yes, they are testing different code paths. TabulatorsTest.testFlatMappingClose tests the closing functionality of Collectors.flatMapping: public static <T, U, A, R> Collector<T, ?, R> flatMapping(Function<? super T, ? extends Stream<? extends U>> mapper, Collector<? super U, A, R> downstream) { BiConsumer<A, ? super U> downstreamAccumulator = downstream.accumulator(); return new CollectorImpl<>(downstream.supplier(), (r, t) -> { try (Stream<? extends U> result = mapper.apply(t)) { if (result != null) result.sequential().forEach(u -> downstreamAccumulator.accept(r, u)); } }, downstream.combiner(), downstream.finisher(), downstream.characteristics()); } Where as the tests of closing in FlatMapOpTest test the closing of Stream.flatMap*: @Override public final <R> Stream<R> flatMap(Function<? super P_OUT, ? extends Stream<? extends R>> mapper) { Objects.requireNonNull(mapper); // We can do better than this, by polling cancellationRequested when stream is infinite return new StatelessOp<P_OUT, R>(this, StreamShape.REFERENCE, StreamOpFlag.NOT_SORTED | StreamOpFlag.NOT_DISTINCT | StreamOpFlag.NOT_SIZED) { @Override Sink<P_OUT> opWrapSink(int flags, Sink<R> sink) { return new Sink.ChainedReference<P_OUT, R>(sink) { @Override public void begin(long size) { downstream.begin(-1); } @Override public void accept(P_OUT u) { try (Stream<? extends R> result = mapper.apply(u)) { // We can do better that this too; optimize for depth=0 case and just grab spliterator and forEach it if (result != null) result.sequential().forEach(downstream); } } }; } }; } >> A following patch, which i plan to fold into the above patch, performs some >> renames on the collectors test to be consistent with the current naming: >> >> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/ > > Renaming looks good and makes it easy to correlate the tests, the assertion > classes, and the collector implementations under test. Unfortunately, the > word for the act of collecting is "collection" which is confusing since > "collection" already has another meaning, but oh well. > Alas :-) Thanks, Paul.
