[ 
https://issues.apache.org/jira/browse/DATAFU-117?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15227447#comment-15227447
 ] 

Matthew Hayes commented on DATAFU-117:
--------------------------------------

Can you open a review board so it is easier to provide feedback?

Some feedback though:
* CountDistinctUpTo.java is missing the standard heading.  You can run "gradle 
rat" to verify.  This isn't automatically run, however it may be reasonable to 
do this.  I'll file a JIRA for that.
* The documented order of parameters isn't consistent with implementation.
* If the input size isn't 2 it should throw an error.
* It's not clear to me why only the first element of the tuples within the bag 
is used.
* You should use "this.set.size() >= maxAmount" instead of "this.set.size() == 
maxAmount" to be safe.
* The condition "this.set.add(o) && (this.set.size() == maxAmount)" seems like 
it could caused the bag to exceed the max size.
* You should have a unit test that uses tuples of more than one element.
* You should have a unit test that calls the accumulate method multiple times.  
I think the lack of this is why the bug I mention above wasn't caught.

> New UDF - CountDistinctUpTo
> ---------------------------
>
>                 Key: DATAFU-117
>                 URL: https://issues.apache.org/jira/browse/DATAFU-117
>             Project: DataFu
>          Issue Type: New Feature
>            Reporter: Eyal Allweil
>         Attachments: DATAFU-117.patch
>
>
> A UDF that counts distinct tuples within a bag, but only up to a preset 
> limit. If the bag contains more distinct tuples than the limit, the UDF 
> returns the limit. 
> This UDF can run reasonably well even on large bags if the limit chosen is 
> small enough though the count is done in memory.
> We use this UDF in PayPal for filtering, when we don't need to use the actual 
> tuples afterward.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to