RFR 8071600: Add a flat-mapping collector

[Stuart Marks]

On 2/11/15 1:54 AM, Paul Sandoz wrote:
>
> 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/

Thanks for the updates. I'm not sure exactly what this webrev represents now, 
but the only change I requested was to change the comment, so no big deal.

>> 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.

Thanks for the explanation.

s'marks

>
> 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.
>