Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14934 )

Change subject: [util] Import FastHash hash function to util
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14934/3/src/kudu/util/hash_util-test.cc
File src/kudu/util/hash_util-test.cc:

http://gerrit.cloudera.org:8080/#/c/14934/3/src/kudu/util/hash_util-test.cc@42
PS3, Line 42: // Test FastHash64/32 returns the expected values for inputs. 
These tests are
> As per smhasher https://github.com/rurban/smhasher, fasthash is similar in
Thank you for the information.

I think at this point it's fine to just add the information on the expected 
speed and other parameters as above into the commit message.  For some reason, 
I was expecting much faster speed compared with murmur2b, but apparently it's 
not the case.


http://gerrit.cloudera.org:8080/#/c/14934/3/src/kudu/util/hash_util.h
File src/kudu/util/hash_util.h:

http://gerrit.cloudera.org:8080/#/c/14934/3/src/kudu/util/hash_util.h@73
PS3, Line 73:   static inline uint64_t FastHash64(const void* buf, size_t len, 
uint64_t seed) {
> Is +1 to Adar's comment or my comment?
That's +1 to Adar's comment.  See 
https://llvm.org/docs/CodingStandards.html#don-t-use-inline-when-defining-a-function-in-a-class-definition
 for details.



--
To view, visit http://gerrit.cloudera.org:8080/14934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0a21d6af10d9ba0dbd9ab46d73552d42976e8d7
Gerrit-Change-Number: 14934
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ban...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Fri, 27 Dec 2019 01:25:47 +0000
Gerrit-HasComments: Yes

Reply via email to