Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’
Patrick Concannon
patrick.concannon at oracle.com
Tue Jul 7 20:46:32 UTC 2020
Hi Anthony,
It's a good point.
You're correct in saying that `Stream.Builder` is a superset of `Consumer`, however `Stream.Builder`'s usual usage is in the creation of new streams. To use it in this context brings with it the connotation that we are going to create a new stream, something we don't want to suggest. In its current state, we push directly downstream and don't create an intermediate stream (like in `flatMap`, for example).
With regards to the build operation, I have to disagree. The build method is central to the `Stream.Builder`'s use case. In your example, I think it's obvious that close shouldn't be called unless you wish the Stream to be closed. However, in the case with build the opposite is true: it's not obvious that a call to build shouldn't be made when you want to finish building your stream.
Both operations are similar, but `Consumer` doesn't bring this baggage with it and therefore seems more fitting.
Kind regards,
Patrick
> On 4 Jul 2020, at 17:23, Anthony Vanelverdinghe <dev at anthonyv.be> wrote:
>
> On 02/07/2020 16:38, Patrick Concannon wrote:
>> Hi Anthony,
> Hi Patrick
>> Thanks for your suggestion of using a Stream.Builder instead of a Consumer.
> Thanks for your response. Since Stream.Builder extends Consumer, I don't see it as "instead of": the proposed signature is a strict superset of the actual signature. For example, it would accept a BiConsumer<Object, Stream.Builder<String>>, whereas the current signature doesn't.
>> However, one of the goals for mapMulti is not to create an additional Stream.
> Would you mind to elaborate on this? Given that Stream.Builder extends Consumer, I don't see why using Stream.Builder would imply having to create any more Streams than when using Consumer.
>> Also, I fear that using a builder, but throwing an exception on a call to build, is counter-intuitive and perhaps confusing to the user.
> Point taken, but I beg to differ: to me this is no different than a method `void foo(Consumer<InputStream> bar)` which specifies that the given Consumer must not invoke `close` on the InputStream.
> Moreover, with the build step being the sole difference with `flatMap`, and the elimination of intermediary Streams being a rather obvious optimization, I believe most users would find it intuitive not to invoke `build` themselves (or they would have read the Javadoc and know not to :)).
> Finally, using Stream.Builder makes it intuitive for me what the method does, even without considering its name.
>
> Given the above, I still hold that Stream.Builder is a better choice to use in the signature. Even more so, I believe there are several things that hint at the need for an additional abstraction: your point about the potential confusion, the analogy with Collector (as already mentioned by Paul w.r.t. the order of the BiConsumer's type arguments), and the natural similarity between e.g. BiConsumer<T, Stream.Builder<R>> and Function<T, Stream<R>>.
>
> Therefore I'd like to propose the following (complete declarations below):
>
> * introduce a new interface: Transformer<T, I, R> extends Function<T, R>
> * adapt Collector to extend it: Collector<T, A, R> extends Transformer<Stream<T>, A, R>
>
> So a Transformer is nothing more than a Function which passes via an intermediary I to map T to R according to a certain protocol.
>
> Then a method with the following signature could be added to Stream:
> <R> Stream<R> flatMap(Transformer<? super T, ? super Stream.Builder<R>, ? extends Stream<R>> mapper)
>
> Such a Transformer could readily be created from a given BiConsumer (see Tranformer::of below).
>
> Since Transformer is a subinterface of Function, there wouldn't be any ambiguities w.r.t. overload resolution, so we could just name the new method `flatMap` (I assume such ambiguities are the reason why this currently isn't the case, since Function and BiConsumer are unrelated?).
>
> Below is a self-contained "demo", containing a simple Demo class, the declaration of Transformer, and the adapted declaration of Collector.
>
> Thanks in advance for any feedback.
>
> Kind regards
> Anthony
>
>
>
> import java.util.ArrayList;
> import java.util.List;
> import java.util.Set;
> import java.util.function.BiConsumer;
> import java.util.function.BinaryOperator;
> import java.util.function.Function;
> import java.util.function.Supplier;
> import java.util.stream.Stream;
>
> public class Demo {
>
> public static void main(String[] args) {
> BiConsumer<String, Stream.Builder<String>> anagrams = (word, builder) -> {
> if(word.equals("listen")) {
> builder.accept("silent");
> builder.accept("enlist");
> }
> };
> Transformer.of(anagrams::accept).apply("listen").forEachOrdered(System.out::println);
> }
>
> }
>
> interface Transformer<T, I, R> extends Function<T, R> {
>
> Supplier<? extends I> supplier();
>
> BiConsumer<? super T, ? super I> transformer();
>
> Function<? super I, ? extends R> finisher();
>
> @Override
> default R apply(T t) {
> var intermediary = supplier().get();
> transformer().accept(t, intermediary);
> return finisher().apply(intermediary);
> }
>
> static <U, V> Transformer<U, Stream.Builder<V>, Stream<V>> of(BiConsumer<? super U, ? super Stream.Builder<V>> transformer) {
> return new Transformer<>() {
> @Override
> public Supplier<Stream.Builder<V>> supplier() {
> return Stream::builder;
> }
>
> @Override
> public BiConsumer<U, Stream.Builder<V>> transformer() {
> return transformer::accept;
> }
>
> @Override
> public Function<Stream.Builder<V>, Stream<V>> finisher() {
> return Stream.Builder::build;
> }
> };
> }
>
> }
>
> interface Collector<T, A, R> extends Transformer<Stream<T>, A, R> {
>
> @Override
> Supplier<A> supplier();
>
> BiConsumer<A, T> accumulator();
>
> BinaryOperator<A> combiner();
>
> @Override
> Function<A, R> finisher();
>
> Set<Characteristics> characteristics();
>
> enum Characteristics {
> CONCURRENT,
> UNORDERED,
> IDENTITY_FINISH
> }
>
> @Override
> default BiConsumer<Stream<T>, A> transformer() {
> return (stream, intermediary) -> {
> var accumulator = accumulator();
> stream.forEachOrdered(t -> accumulator.accept(intermediary, t));
> };
> }
>
> }
>
>>
>> Kind regards,
>>
>> Patrick
>>
>>> On 25 Jun 2020, at 18:12, Anthony Vanelverdinghe <dev at anthonyv.be> <mailto:dev at anthonyv.be> wrote:
>>>
>>> Hi
>>>
>>> Given the signature of flatMap is:
>>> <R> Stream<R> flatMap(Function<? super T, ? extends Stream<? extends R>> mapper)
>>>
>>> I'd like to propose the following signature for the new method:
>>> <R> Stream<R> builderMap(BiConsumer<? super T, ? super Stream.Builder<R>> mapper)
>>>
>>> This way both methods are named "...Map", and the name "builderMap" follows naturally from the argument's type.
>>> If the given mapper invokes Stream.Builder::build, an IllegalStateException should be thrown.
>>>
>>> Kind regards,
>>> Anthony
>>>
>>> On 25/06/2020 02:58, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> We traded CPS style for reusing existing functional interfaces. Originally the signature (my first choice) was as you indicate.
>>>>
>>>> By chance it just so happens that the current signature is the same shape as that for the accumulating argument type of the three arg collect terminal operation:
>>>>
>>>> Stream
>>>> <R> R collect(Supplier<R> supplier,
>>>> BiConsumer<R, ? super T> accumulator,
>>>> BiConsumer<R, R> combiner);
>>>>
>>>> IntStream
>>>> <R> R collect(Supplier<R> supplier,
>>>> ObjIntConsumer<R> accumulator,
>>>> BiConsumer<R, R> combiner);
>>>>
>>>> Same for the accumulator of a Collector too.
>>>>
>>>> However, I suspect you would argue these terminal accumulation cases are different from the intermediate case, as we are not accumulating but passing or accepting (loosely returning) zero or more elements that replace the input element.
>>>>
>>>> It’s my hope that generic specialization will allow the primitive stream types to fade into the background, along with the primitive functional interfaces. In that respect the addition of three functional interfaces for use on the primitive stream types is not so terrible.
>>>>
>>>>
>>>> Regarding the name, you should have seen the first one :-) it was terrible.
>>>>
>>>> Here’s my few brush strokes on the bike shed. I wonder what people think of mapAccept. The specification talks about accepting elements, because that is the operative method name on Consumer. So we can say "T is replaced with the elements accepted by the Consumer<R>", or “ The Consumer<R> accepts the elements that replace T"
>>>>
>>>> Paul.
>>>>
>>>>
>>>>
>>>>> On Jun 24, 2020, at 1:01 PM, John Rose <john.r.rose at oracle.com> <mailto:john.r.rose at oracle.com> wrote:
>>>>>
>>>>> I like this new API point a lot; it allows flexible, local, temporary
>>>>> control inversion in the context of one stream transform.
>>>>>
>>>>> What’s the performance model? It seems like the SpinedBuffer
>>>>> implementation makes a worst-case assumption about the number
>>>>> of pending values, that there will be many instead of zero or one.
>>>>>
>>>>> But I guess the pipeline stuff already works in terms of pushes, so
>>>>> the good news might be that this is really just a drill-down from the
>>>>> user API into the kinds of operations (push-flavored) that go on
>>>>> most of the time.
>>>>>
>>>>> OK, so I like the function but I have a beef with its bike shed
>>>>> color. First of all, this is a control-inversion (CPS) pattern,
>>>>> which is very powerful but also notoriously hard to read.
>>>>> I think that in Java APIs, at least in Stream APIs, code is
>>>>> easier to read if the logical data flow is from left to right.
>>>>>
>>>>> (This is a language-specific observation. Apart from varargs,
>>>>> Java method APIs read favorably when extra control arguments
>>>>> are added onto the end of the argument list. Also, the convention
>>>>> for generic functional interfaces is that the return value type
>>>>> goes to the right, e.g., R in Function<A,R>.)
>>>>>
>>>>> So the BiConsumer is backwards, because the logical return
>>>>> should be written, if not as a true return (which would appear
>>>>> at the end of type parameter lists), at the end of the incoming
>>>>> parameters (and in the last type parameter).
>>>>>
>>>>> I also think “multi” is needlessly “learned” sounding. A simple
>>>>> spatial preposition would work well: mapThrough, mapAcross, etc.
>>>>> I think I prefer mapAcross because the term “across” can be read
>>>>> adverbially: “we are mapping T across to Consumer<R>”.
>>>>>
>>>>> So:
>>>>>
>>>>> mapAcross(BiConsumer<? super T, Consumer<R>> mapper)
>>>>> mapAcrossToInt(BiConsumer<? super T, IntConsumer> mapper)
>>>>> mapAcross(IntObjConsumer<IntConsumer> mapper)
>>>>>
>>>>> This does require additional FI’s like IntObjConsumer, but
>>>>> I think that is a negligible cost. Making the control inversion
>>>>> *readable* is the high order bit here, not minimizing the number
>>>>> of trivial FIs.
>>>>>
>>>>> (I almost hear David Bowman, in his space suit, saying, “My API…
>>>>> It’s full of bikesheds!” There’s a meme for that.)
>>>>>
>>>>> — John
>>>>>
>>>>> On Jun 24, 2020, at 3:57 AM, Patrick Concannon <patrick.concannon at oracle.com> <mailto:patrick.concannon at oracle.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Could someone please review myself and Julia's RFE and CSR for JDK-8238286 - 'Add new flatMap stream operation that is more amenable to pushing’?
>>>>>>
>>>>>> This proposal is to add a new flatMap-like operation:
>>>>>>
>>>>>> `<R> Stream<R> mapMulti(BiConsumer<Consumer<R>, ? super T> mapper)`
>>>>>>
>>>>>> to the java.util.Stream class. This operation is more receptive to the pushing or yielding of values than the current implementation that internally assembles values (if any) into one or more streams. This addition includes the primitive variations of the operation i.e. mapMultiToInt, IntStream mapMulti, etc.
>>>>>>
>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8238286 <https://bugs.openjdk.java.net/browse/JDK-8238286> <https://bugs.openjdk.java.net/browse/JDK-8238286> <https://bugs.openjdk.java.net/browse/JDK-8238286>
>>>>>> csr: https://bugs.openjdk.java.net/browse/JDK-8248166 <https://bugs.openjdk.java.net/browse/JDK-8248166> <https://bugs.openjdk.java.net/browse/JDK-8248166> <https://bugs.openjdk.java.net/browse/JDK-8248166>
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/ <http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/> <http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/> <http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/>
>>>>>> specdiff: http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html <http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html> <http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html> <http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html>
>>>>>>
>>>>>>
>>>>>> Kind regards,
>>>>>> Patrick & Julia
More information about the core-libs-dev
mailing list