Hi Tagir, thanks for the update.
Also thanks Tomasz for keeping everybody honest on the open issues.
First, naming. I think "duplex" as the root word wins! Using "duplexing" to
conform to many of other collectors is fine; so, "duplexing" is good.
Unfortunately "duplex" is not really a verb. For some reason "duplexing" sounds
OK. This probably follows in the long tradition of the tech industry's "verbing"
of nouns and adjectives. However, I can't quite bring myself to recommend using
this in the javadoc summary sentence, e.g.,
Returns a {@code Collector} that duplexes stream elements to two downstream
collectors.
Ugh. I think the current summary sentence is fine
Returns a {@code Collector} that is a composite of two downstream collectors.
even though it uses "composite" which is not the name of this collector. But I
think it's a more descriptive term and it reads more smoothly.
Turning to the issues mentioned by Tomasz:
1) Brian Goetz' suggestion of changing "? extends R" into "R":
- http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054947.html
Yes, I think this should be done.
2) Stuart Marks' suggestion about renaming "c1" and "c2" to "downstream1" and
"downstream2":
- http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054949.html
Yes. It's a small thing, but the parameter names do appear in the javadoc. The
text refers to "downstream" collectors, and naming the parameters "downstream"
strengthens the association to the text. Otherwise, the reader has to think
"what are c1 and c2? Oh, they're the downstream collectors."
3) my suggestion about renaming "merger" to "biFinisher" because "merger"
means BinaryOperator everywhere else:
-
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055235.html
I'm ok with "merger". I don't feel that "merge" has a strong implication that
the result must be the same type as the two inputs. I'm ok with the notion of
"merge" taking two inputs (which might be of different types) and producing a
single output (which might be a different type from either input). The
difficulty with "biFinisher" is that it doesn't seem to imply "take two things
and produce one". A finisher takes one input and produces one output, so would a
"biFinisher" take two inputs and produce two outputs?
As to the name, I still think a name related to a well-understood concept
(like the composite pattern [1]) would be better. Note that "composite" is
also a verb [2], but "Collectors.compositing" looks a bit strange.
"Collectors.composing" seems much better to me, but - as far as I understand -
there was some concern that the users could misunderstand it as element-wise
composition, is that right?
The difficulty I have with "compose" and "composition" is that with function
composition, one function is applied, and the second function is applied to the
result of the first. Of course that isn't what happens here. The notion of
"duplexing" is that things are happening side-by-side, which applies well here,
I think.
**
Tagir, overall, looks good! Let me know when you've finished updating the CSR.
Thanks,
s'marks
Regards,
Tomasz Linkowski
[1] https://en.wikipedia.org/wiki/Composite_pattern
[2] https://en.wiktionary.org/wiki/composite#Verb
On Fri, Sep 14, 2018 at 11:23 AM, Peter Levart <peter.lev...@gmail.com
<mailto:peter.lev...@gmail.com>> wrote:
Hi Tagir,
I like duplexing more than teeingAndThen. If consensus can be established
about the name, I think you will then want to update the CSR draft to
reflect new name. Then we'll kindly ask Stuart if he has any more advice
before submitting the CSR...
Regards, Peter
On 09/14/2018 10:41 AM, Tagir Valeev wrote:
Hello, Stuart and Peter!
Thank you for valuable comments. I updated the webrev:
http://cr.openjdk.java.net/~tvaleev/webrev/8205461/r4/
<http://cr.openjdk.java.net/%7Etvaleev/webrev/8205461/r4/>
1. I renamed "teeingAndThen" to "duplexing". Brian insisted that
"-ing" suffix shall present and I agree. Hopefully it's final name.
2. I updated the spec as Stuart suggested.
No changes in implementation since r3 revision. Please check.
With best regards,
Tagir Valeev.
On Tue, Aug 21, 2018 at 12:43 PM Stuart Marks <stuart.ma...@oracle.com
<mailto:stuart.ma...@oracle.com>> wrote:
Hi Tagir,
Thanks for working on this. This looks really cool! And thanks
Peter for
agreeing to sponsor it.
I can help out with the CSR. My first bit of advice about the CSR
process is to
hold off until the specification is complete. :-)
I think the intent of the API is fine, but I think some details of
the returned
collector need to be ironed out first.
1. The spec doesn't say what the returned collector's supplier,
accumulator,
combiner, and finisher do. On the one hand, we don't necessarily
want to
describe the actual implementation. On the other hand, we need to
specify how
the thing actually behaves. One can certainly deduce the intended
behavior from
the description, but this really needs to be specified, and it
mustn't rely on
the reader having to derive the required behaviors. Since the actual
implementation is fairly simple, the spec might end up being
rather close to the
implementation, but that might be unavoidable.
I'm envisioning something like this:
- supplier: creates a result container that contains result
containers
obtained by calling each collector's supplier
- accumulator: calls each collector's accumulator with its
result container
and the input element
... and similar for the combiner and finisher functions.
2. Characteristics
- UNORDERED: should the returned collector be UNORDERED if
*either* of the
provided collectors is UNORDERED? (Current draft says *both*.)
- CONCURRENT: current draft seems correct
- IDENTITY_FINISH: clearly not present; perhaps this should be
specified
3. Parameter naming
The collector parameters are referred to as "specified collectors"
or "supplied
collectors". Other "higher-order" collectors refer to their
underlying
collectors as "downstream" collectors. I think it would be useful
to work the
"downstream" concept into this collector. This would enable the
opening summary
sentence of the doc to be something like, "Returns a collector
that is a
composite of two downstream collectors" or some such. (But see
naming below.)
4. Naming
Sigh, naming is hard, and I know there was a fair amount of
discussion in the
previous thread and earlier in this one, but it seems like there's
still some
dissatisfaction with the name. (And I'm not particularly thrilled
with
teeingAndThen myself.) In a few minutes I've managed to come up
with a few more
names that (mostly) don't seem to have been proposed before, and
so here they
are for your consideration:
- compound
- composite
- conjoined
- bonded
- fused
- duplex
Discussion:
A "composite" evokes function composition; this might be good,
though it might
be odd in that collectors can't be composed in the same way that
functions are.
"Compound" might be a useful alternative. In chemistry, two
substances are
combined (or bonded, or fused) to form a single substance, which
is a compound.
"Conjoin" seems to adequately describe the structure of the two
collectors, but
it lacks somewhat the connotation of unifying them.
In an earlier discussion, Brian had pushed back on names related to
split/fork/merge/join since those are currently in use in streams
regarding
splitting of input elements and merging of results. In describing
how the
current proposal differs, he said that elements are "multiplexed"
to the
different collectors. Since we're doing this with two collectors,
how about
"duplex"? (I note that Jacob Glickman also had suggested "duplex".)
s'marks
On 8/20/18 1:48 AM, Tagir Valeev wrote:
Hello!
A CSR is created:
https://bugs.openjdk.java.net/browse/JDK-8209685
<https://bugs.openjdk.java.net/browse/JDK-8209685>
(this is my first CSR, hopefully I did it correctly)
With best regards,
Tagir Valeev.
On Mon, Aug 20, 2018 at 2:06 PM Peter Levart
<peter.lev...@gmail.com <mailto:peter.lev...@gmail.com>> wrote:
Hi Tagir,
I think this looks very good. It just needs a CSR. Will
you file it?
Regards, Peter
On 08/19/2018 11:24 AM, Tagir Valeev wrote:
Hello, Brian!
Of the three phases, teeing is the most important and
least obvious, so
I think something that includes that in the name is going
to be
helpful. Perhaps "teeingAndThen" is more evocative and
not totally
unwieldy.
Ok, sounds acceptable to me. Renamed pairing to
teeingAndThen.
By the way looking into CollectorsTest.java I found some
minor things to
cleanup:
1. `.map(mapper::apply)` and `.flatMap(mapper::apply)` can
be replaced with
simple `.map(mapper)` and `.flatMap(mapper)` respectively
Does IntelliJ have an inspection for eliminating such
locutions?
Sure, that's how I found them. Well, I took the liberty to
fix these two things.
2. In many methods redundant `throws
ReflectiveOperationException` is
declared while exception is never thrown
For test code where a significant fraction of test cases
are going to
throw something, we often do this, since its easier to
just uniformly
tag such methods rather than thinking about which test
methods actually
throw the exception and which don't. So I think this is
harmless
(though cleaning it up is harmless too.)
I'm not thinking about this, because my IDE thinks for me
:-) Ok, I'll
leave them as is for now.
You may want to optimize the EnumSet mechanics for the
case where
neither collector has interesting characteristics.
Added a special case when reported characteristics for
either of
collectors are empty or IDENTITY_FINISH only.
I think this should be a common case.
The updated webrev is posted here (along with Peter
suggestion to
rename finisher to merger):
http://cr.openjdk.java.net/~tvaleev/webrev/8205461/r3/
<http://cr.openjdk.java.net/%7Etvaleev/webrev/8205461/r3/>
Also copyright year is updated
With best regards,
Tagir Valeev
--
Pozdrawiam,
Tomasz Linkowski
