[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has submitted this change and it was merged. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() This change implements support for TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue(). TimestampValue itself is 16 bytes in size. To match RawValue::Write() in the interpreted path, CodegenAssignNullValue() emits code to assign HashUtil::FNV_SEED to both the upper and lower 64-bit of the destination value. This change also fixes the handling of 128-bit Decimal16Value in CodegenAssignNullValue() so the emitted code matches the behavior of the interpreted path. Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Reviewed-on: http://gerrit.cloudera.org:8080/4794 Reviewed-by: Michael Ho Tested-by: Michael Ho --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/hash-table.cc M be/src/exec/old-hash-table.cc M be/src/util/bit-util.h M testdata/workloads/functional-query/queries/QueryTest/joins.test 7 files changed, 72 insertions(+), 37 deletions(-) Approvals: Michael Ho: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 3: Verified+1 -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 3: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/344/ -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has uploaded a new patch set (#3). Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() This change implements support for TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue(). TimestampValue itself is 16 bytes in size. To match RawValue::Write() in the interpreted path, CodegenAssignNullValue() emits code to assign HashUtil::FNV_SEED to both the upper and lower 64-bit of the destination value. This change also fixes the handling of 128-bit Decimal16Value in CodegenAssignNullValue() so the emitted code matches the behavior of the interpreted path. Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/hash-table.cc M be/src/exec/old-hash-table.cc M be/src/util/bit-util.h M testdata/workloads/functional-query/queries/QueryTest/joins.test 7 files changed, 72 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/4794/3 -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 3: Code-Review+2 Carry Tim's +2 forward. -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4794 to look at the new patch set (#3). Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() This change implements support for TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue(). TimestampValue itself is 16 bytes in size. To match RawValue::Write() in the interpreted path, CodegenAssignNullValue() emits code to assign HashUtil::FNV_SEED to both the upper and lower 64-bit of the destination value. This change also fixes the handling of 128-bit Decimal16Value in CodegenAssignNullValue() so the emitted code matches the behavior of the interpreted path. Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/hash-table.cc M be/src/exec/old-hash-table.cc M be/src/util/bit-util.h M testdata/workloads/functional-query/queries/QueryTest/joins.test 7 files changed, 72 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/4794/3 -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4794/2/be/src/util/bit-util.h File be/src/util/bit-util.h: PS2, Line 85: conste > constexpr - that way we can use it in static_asserts() etc in future. Done -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/4794/2/be/src/util/bit-util.h File be/src/util/bit-util.h: PS2, Line 85: static constexpr - that way we can use it in static_asserts() etc in future. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has posted comments on this change. Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4794/2/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS2, Line 386: repsectively typo: respectively -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
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 Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has uploaded a new patch set (#2). Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() This change implements support for TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue(). TimestampValue itself is 16 bytes in size. To match RawValue::Write() in the interpreted path, CodegenAssignNullValue() emits code to assign HashUtil::FNV_SEED to both the upper and lower 64-bit of the destination value. This change also fixes the handling of 128-bit Decimal16Value in CodegenAssignNullValue() so the emitted code matches the behavior of the interpreted path. Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/hash-table.cc M be/src/exec/old-hash-table.cc M be/src/util/bit-util.h M testdata/workloads/functional-query/queries/QueryTest/joins.test 7 files changed, 72 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/4794/2 -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
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 Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3884: Support TYPE TIMESTAMP for HashTableCtx::CodegenAssignNullValue()
Michael Ho has uploaded a new change for review. http://gerrit.cloudera.org:8080/4794 Change subject: IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() .. IMPALA-3884: Support TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue() This change implements support for TYPE_TIMESTAMP for HashTableCtx::CodegenAssignNullValue(). TimestampValue itself is 16 bytes in size. To match RawValue::Write() in the interpreted path, CodegenAssignNullValue() emits code to assign HashUtil::FNV_SEED to both the upper and lower 64-bit of the destination value. This change also fixes the handling of 128-bit Decimal16Value in CodegenAssignNullValue() so the emitted code matches the behavior of the interpreted path. Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 --- M be/src/codegen/llvm-codegen.cc M be/src/codegen/llvm-codegen.h M be/src/exec/hash-table.cc M testdata/workloads/functional-query/queries/QueryTest/joins.test 4 files changed, 44 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/4794/1 -- To view, visit http://gerrit.cloudera.org:8080/4794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0211d38cbef46331e0006fa5ed0680e6e0867bc8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho