Re: Review Request 21618: DATAFU-47 UDF for Murmur3 (and other) Hash functions

[Eyal Allweil via Review Board <no...@reviews.apache.org>]

> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/Hasher.java
> > Lines 90 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584924#file584924line90>
> >
> >     Shouldn't this be like below instead?
> >     
> >     this("murmur3-32")

Fixed - looks like the default Guava constructor uses this value anyway.


> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/Hasher.java
> > Lines 126 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584924#file584924line126>
> >
> >     Comment is wrong.  There is no seed.

I removed the mention of the seed in the comment.


> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/Hasher.java
> > Lines 190 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584924#file584924line190>
> >
> >     remove extra line

Removed.


> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/Hasher.java
> > Lines 193 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584924#file584924line193>
> >
> >     Error includes all hash functions, where here it should only include those that take seeds.

Error message changed.


> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/HasherRand.java
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584925#file584925line30>
> >
> >     Needs doc string, which should distinguish this from Hasher.

I changed the javadoc a bit, and added a reference to Hasher to make it easier to understand.


> On July 14, 2014, 8:43 p.m., Matthew Hayes wrote:
> > datafu-pig/src/main/java/datafu/pig/hash/HasherRand.java
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/21618/diff/3/?file=584925#file584925line46>
> >
> >     Should we have a default constructor of murmur3-32 like Hasher?

Added one. Makes sense to me.


- Eyal


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21618/#review47722
-----------------------------------------------------------


On May 20, 2014, 12:19 p.m., Philip (flip) Kromer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21618/
> -----------------------------------------------------------
> 
> (Updated May 20, 2014, 12:19 p.m.)
> 
> 
> Review request for DataFu and Matthew Hayes.
> 
> 
> Bugs: DATAFU-47
>     https://issues.apache.org/jira/browse/DATAFU-47
> 
> 
> Repository: datafu
> 
> 
> Description
> -------
> 
> Accompanies DATAFU-47 https://issues.apache.org/jira/browse/DATAFU-47 -- make sure to apply the patch from DATAFU-46 too first
> 
> Questions for reviewers:
> 
> * If we upgrade Guava, we'd get sip24 (a fast cryptographically secure hash), crc32 and adler32 (occasionally useful checksums). I can put the update in as another patch. Should we upgrade?
> * This UDF provides the same hashes as MD5 and SHA udfs. Should those be deprecated in favor of this? I can add the binhex functionality so that nothing is lost.
> * If there's a standard way to do the dependency injection of a fixed random number generator for the tests please advise.
> 
> 
> Diffs
> -----
> 
>   datafu-pig/src/main/java/datafu/pig/hash/Hasher.java PRE-CREATION 
>   datafu-pig/src/main/java/datafu/pig/hash/HasherRand.java PRE-CREATION 
>   datafu-pig/src/test/java/datafu/test/pig/hash/HashTests.java 7ff8fb9 
>   datafu-pig/src/test/java/datafu/test/pig/hash/HasherRandForTesting.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/21618/diff/3/
> 
> 
> Testing
> -------
> 
>  ./gradlew :datafu-pig:test -Dtest.single=HashTests 
> 
> 
> Thanks,
> 
> Philip (flip) Kromer
> 
>