Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4794/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 586: DCHECK_EQ(num_bytes & (num_bytes - 1), 0); > Could you factor this into BitUtil::IsPowerOfTwo() or something along those Done Line 591: return ConstantInt::get(context(), APInt(128, vals)); > CodegenAnyVal::SetVal(int128_t) should use this approach. There is a TODO i Done http://gerrit.cloudera.org:8080/#/c/4794/1/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 387: /// than 64-bit), an 128-bit value is formed by setting its upper and lower 64-bit set > This behaviour of copying the value into the upper and lower bits is pretty Thanks for the suggestion. Refactored GetIntConstant() to take two 64-bit values instead. APInt() will handle masking out appropriate bits and getting the right size. http://gerrit.cloudera.org:8080/#/c/4794/1/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 597: // fall through to generate 128-bit 'fnv_seed' > May as well just call codegen->GetIntConstant() here, or codegen->GetInt128 Done PS1, Line 608: fnv_seed_float > Thank you! This bugged me when reading the code before. Optional, but you c Done http://gerrit.cloudera.org:8080/#/c/4794/1/testdata/workloads/functional-query/queries/QueryTest/joins.test File testdata/workloads/functional-query/queries/QueryTest/joins.test: Line 723: ---- QUERY > I have a test in https://gerrit.cloudera.org/#/c/4655/6/tests/query_test/te Thanks for the heads-up. -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
