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