[
https://issues.apache.org/jira/browse/CRUNCH-484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14258347#comment-14258347
]
Gabriel Reid commented on CRUNCH-484:
-------------------------------------
Sorry for the super-slow feedback loop [~davw], but looking at this today makes
it feel like a nice Christmas present :-) Really, this is very very awesome.
My opinons on your open questions:
{quote}Does all of this belong here?{quote}
If you ask me, yes, absolutely. All very useful and generic enough to belong
here.
{quote}Should Percentiles be renamed to Quantiles, given that you actually just
specify a 0.0-1.0 range? Quantiles is more accurate, but people are more likely
to search for Percentiles.{quote}
FWIW, I think I would actually search for Quantiles. The main reason to leave
it as Percentiles is to ease the transition from using this stuff in
crunch-lib.
{quote}Would the DoFns.detach(...) functionality be better implemented as a
DetachingReduceFn base class rather than a wrap-and-delegate?{quote}
I prefer it how it is. The only reason I see to have it as a base class is that
it makes it a bit easier to explain what it does exactly. However, I think that
the advantages of avoiding inheritance are worth it here.
BTW, I noticed you check for a null Configuration in DetachingDoFn.initialize.
Is this a situation that you've actually come up against? As far as I know, the
only way this can happen is with a misbehaving class that is wrapping a DoFn.
Not something that needs to be taken care of as part of this ticket, but it
would be good to know if that's something worth looking into further.
Also, one tiny nit: I saw there are some wildcard imports such as "import
java.util.*" in DoFns.java. In the non-existent Crunch code style guide it's
specified to avoid this.
> Add library features from spotify/crunch-lib into crunch-core
> -------------------------------------------------------------
>
> Key: CRUNCH-484
> URL: https://issues.apache.org/jira/browse/CRUNCH-484
> Project: Crunch
> Issue Type: Improvement
> Components: Core
> Reporter: David Whiting
> Assignee: Josh Wills
> Attachments:
> 0001-CRUNCH-484-Add-library-features-from-spotify-crunch-.patch
>
>
> As suggested by Josh, I've patched the more generally-applicable stuff from
> https://github.com/spotify/crunch-lib into org.apache.crunch.lib
> Open questions:
> - Does all of this belong here?
> - Should Percentiles be renamed to Quantiles, given that you actually just
> specify a 0.0-1.0 range? Quantiles is more accurate, but people are more
> likely to search for Percentiles.
> - Would the DoFns.detach(...) functionality be better implemented as a
> DetachingReduceFn base class rather than a wrap-and-delegate?
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
