Add sortedTakeBy and sortedReverseTakeBy to Aggregator.scala

[joshualande]

You can accomplish the same thing by passing Ordering.by(fn) directly to the sortedTake function, but I think adding these new functions makes the code a bit easier to use and will make downstream code more readable.

I didn’t see any existing unit tests on Aggregator.sortedTake, so I didn’t add any tests to this code. But would be willing to add some additional tests if we feel it is necessary.

[joshualande]

Fixing a merging/pushing issue. I think the PR should be good now.

[avibryant]

typo ("sorteTake")

[avibryant]

These seem fine, but I wonder if the most usable API would be to add sort and sortBy to Preparer - I'm imagining that this would return some SortedPreparer object that had methods like head, last, reverse, and take (each of which would produce an Aggregator).

[joshualande]

@avibryant

I fixed the typo! Great catch.

With your alternate API suggestion, is the idea that the code to build an equivalent function would be:

Preparer.sortBy[Type](_.timestamp).take(10)

vs

Aggregator.sortedTakeBy[Type, Long](10)(_.timestamp)

If so, would we then make Aggregator.sortedTakeBy a thin wrapper around the Preparer code?

One last question is, wouldn't it make more sense to call the functions in SortedPreparer 'max' and 'min' instead of 'head' and tail for clarity?

Thanks for your help!!!

[johnynek]

can we call this sortByTake mirroring the style of using scala names? This is like sortBy(fn).take(count)

[johnynek]

+1 to add to the Preparer, but I would leave your implementations as is (just call the corresponding sortedTake method).

[johnynek]

@joshualande can you branch this diff off develop? I think you have master here, which is why you have all these tags and merges mentioned

[joshualande]

@johnynek great catch. I messed it the merge, then fixed it, then messed it up again, but I think its all good now :) Still getting the hang of github PRs!

I updated the function names and comments. I would be happy to try the Preparing approach later when I've got more free time, but do you think it would make more sense to ship this first since the API seems pretty straightforward?

[avibryant]

@joshualande I think it would actually be Preparer[Type].sortBy(_.timestamp).take(10) but yes, that's what I was thinking.

Re: min vs head. I guess once I think of it as a sorted list, then head makes more sense and is more consistent with what we do on TypedPipe. But I can see preferring maxBy to sortBy followed by head.

[joshualande]

@avibryant @johnynek thanks so much for your help!!! I really appreciate your suggestions, and it is great to have a chance to contribute to the codebase!

I'll give this Preparer code a try when I've got a bit more free time!!

[johnynek]

👍