Re: Behavior of Top.Largest

2017-05-22 Thread Kenneth Knowles
On Mon, May 22, 2017 at 10:47 AM, Robert Bradshaw < rober...@google.com.invalid> wrote: > > For a simple rename like this, I would at least strongly encourage > fixing both for those that are capable. > Agreed. My response suffered from premature generalization, wandering off into "policy"-esque t

Re: Behavior of Top.Largest

2017-05-22 Thread Robert Bradshaw
On Mon, May 22, 2017 at 10:24 AM, Kenneth Knowles wrote: > There was a brief discussion on a JIRA about following the pattern like: > > Main task: API change proposal > - subtask: Java change > - subtask: Python change > - subtask: ... etc ... We should at least do this. > This allows asynchr

Re: Behavior of Top.Largest

2017-05-22 Thread Robert Bradshaw
On Sun, May 21, 2017 at 11:45 AM, Dan Halperin wrote: > I think this is an unrealistic request -- Python and Java workflows are > completely different, and Python developer documentation is especially > abysmal. We should improve this for sure. > (E.g., I had to have Robert sit with me to get th

Re: Behavior of Top.Largest

2017-05-22 Thread Kenneth Knowles
There was a brief discussion on a JIRA about following the pattern like: Main task: API change proposal - subtask: Java change - subtask: Python change - subtask: ... etc ... This allows asynchrony without losing track of our intent. Of course, I hope Beam is so successful that we have too man

Re: Behavior of Top.Largest

2017-05-21 Thread Dan Halperin
I think this is an unrealistic request -- Python and Java workflows are completely different, and Python developer documentation is especially abysmal. (E.g., I had to have Robert sit with me to get the Python SDK to work at all on my developer machine, and even then I gave up and chmod-ed my mach

Re: Behavior of Top.Largest

2017-05-19 Thread Ahmet Altay
I mentioned this in the PR, I believe it is worth repeating here. It is important to keep the API consistent between Python and Java. It would help a lot, if changes are applied to both SDKs at the same time. If that is not possible, an easier alternative would be to file a JIRA issue so that the

Re: Behavior of Top.Largest

2017-05-19 Thread Robert Bradshaw
I see this was implemented. Do we have a policy/guideline for when a name is "bad enough" to merit renaming (and keeping a duplicate, deprecated member around for a year or more). On Mon, May 15, 2017 at 9:25 AM, Robert Bradshaw wrote: > On Sun, May 14, 2017 at 3:36 PM, Ben Chambers > wrote: >>

Re: Behavior of Top.Largest

2017-05-15 Thread Robert Bradshaw
On Sun, May 14, 2017 at 3:36 PM, Ben Chambers wrote: > Exposing the CombineFn is necessary for use with composed combine or > combining value state. There may be other reasons we made this visible, but > these continue to justify it. > These are the CompareFns, not the CombineFns. It'd be nicer

Re: Behavior of Top.Largest

2017-05-14 Thread Reuven Lax
Makes sense - however a bit sad that it then got reused by other unrelated combiners (e.g. ApproximateQuantiles). Could we replace the internal uses of this class with com.google.common.collect.Ordering.natural? On Sun, May 14, 2017 at 3:36 PM, Ben Chambers wrote: > Exposing the CombineFn is nec

Re: Behavior of Top.Largest

2017-05-14 Thread Ben Chambers
Exposing the CombineFn is necessary for use with composed combine or combining value state. There may be other reasons we made this visible, but these continue to justify it. On Sun, May 14, 2017, 1:00 PM Reuven Lax wrote: > I believe the reason why this is called Top.largest, is that originally

Re: Behavior of Top.Largest

2017-05-14 Thread Reuven Lax
I believe the reason why this is called Top.largest, is that originally it was simply the comparator used by Top.largest - i.e. and implementation detail. At some point it was made public and used by other transforms - maybe making an implementation detail a public class was the real mistake? On S

Re: Behavior of Top.Largest

2017-05-14 Thread Davor Bonaci
I agree this is an unfortunate name. Tangential: can we rename APIs now that the first stable release is nearly done? Of course -- the "rename" can be done by introducing a new API, and deprecating, but not removing, the old one. Then, once we decide to move to the next major release, the deprecat

Behavior of Top.Largest

2017-05-13 Thread Wesley Tanaka
Using Top.Largest to sort a list of {2,1,3} produces {1,2,3}.  This matches the javadoc for the class, but seems counter-intuitive -- one might expect that a Comparator called Largest would give largest items first.  I'm wondering if renaming the classes to Natural / Reversed would better match