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 <jinc...@gmail.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 03 Jan 2018 16:58:50 +0000 Gerrit-HasComments: Yes