crunch-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabriel Reid (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CRUNCH-484) Add library features from spotify/crunch-lib into crunch-core
Date Wed, 24 Dec 2014 15:58:13 GMT

    [ 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)

Mime
View raw message