[Impala-ASF-CR] IMPALA-3651: Adds murmur hash() built-in function

["Tim Armstrong (Code Review)" <ge...@cloudera.org>]

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8893 )

Change subject: IMPALA-3651: Adds murmur_hash() built-in function
......................................................................


Patch Set 2:

(3 comments)

Looks good, just a few minor requests.

http://gerrit.cloudera.org:8080/#/c/8893/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8893/2//COMMIT_MSG@13
PS2, Line 13: Add unit tests for primitive types
Can you add a test query to exprs.test, just to make sure it works end-to-end?


http://gerrit.cloudera.org:8080/#/c/8893/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8893/2/be/src/exprs/expr-test.cc@4564
PS2, Line 4564:   // Test murmur_hash
nit: comment is not really needed. Maybe it would actually be best to make the new tests a separate TEST_F function?


http://gerrit.cloudera.org:8080/#/c/8893/2/be/src/exprs/expr-test.cc@4571
PS2, Line 4571:     expected = HashUtil::MurmurHash2_64(s.data(), s.size(), HashUtil::MURMUR_DEFAULT_SEED);
In the cases where we're hashing a constant, can you add a test that compares to a constant? Just so we detect if MurmurHash2_64 accidentally changes behaviour?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 16:58:50 +0000
Gerrit-HasComments: Yes