RFR 8071600: Add a flat-mapping collector

[Paul Sandoz]

On Feb 11, 2015, at 12:02 AM, Stuart Marks <stuart.marks at 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.