impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Date Sat, 22 Oct 2016 03:19:45 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue()

Patch Set 1:

File be/src/codegen/

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.
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
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".
File be/src/exec/

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
File testdata/workloads/functional-query/queries/QueryTest/joins.test:

Line 723: ---- QUERY
I have a test in
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message