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