Tim Armstrong 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 lines. That would be generally useful - e.g. I know some cases in the buffer pool code where I'd use it. Line 591: return ConstantInt::get(context(), APInt(128, vals)); CodegenAnyVal::SetVal(int128_t) should use this approach. There is a TODO in there. 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 confusing - I would expect it to return an int128_t with all the upper bits 0. E.g. GetIntConstant(byte_size, 1) should always return 1 regardless of the byte_size. Maybe we should have a separate GetInt128Constant(int bytes_size, int64_t upper, int64_t lower) method that does this so the behaviour is more "obvious". 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->GetInt128Constant() here. It only saves a line of code. PS1, Line 608: fnv_seed_float Thank you! This bugged me when reading the code before. Optional, but you could do the search-and-replace too in old-hash-table.cc 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/test_aggregation.py that disables the check if codegen is enabled for timestamps. Depending on which patch lands first we should make sure that's reenabled. -- 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 <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
