Conversation
Devjiu
force-pushed
the
dmitriim/init_hash_table_cpu_opt
branch
from
164d7d5 to
c6c7e7a
Compare
| data_provider, | ||
| column_cache); | ||
| if (col_buff != nullptr) { | ||
| join_chunk_array[num_chunks] = JoinChunk{col_buff, elem_count, num_elems}; |
There was a problem hiding this comment.
Should we force to initialize all fields with constructor?
Devjiu
force-pushed
the
dmitriim/init_hash_table_cpu_opt
branch
2 times, most recently
from
74c47ff to
ac76362
Compare
ienkovich
left a comment
ienkovich
left a comment
There was a problem hiding this comment.
The patch looks good! There are a few minors to fix though.
| #include "../../DecodersImpl.h" | ||
| #else | ||
| #include "../../RuntimeFunctions.h" | ||
| #include "HashJoinRuntimeCpu.h" |
There was a problem hiding this comment.
This header doesn't seem to be used in this file.
| namespace { | ||
|
|
||
| template <ColumnType T> | ||
| inline int64_t getElem(const int8_t* chunk_mem_ptr, size_t elem_size, size_t elem_ind) { |
There was a problem hiding this comment.
You better get an error on compile time rather than runtime by using
template <ColumnType T>
inline int64_t getElem(const int8_t* chunk_mem_ptr, size_t elem_size, size_t elem_ind) = delete;
| @@ -0,0 +1,261 @@ | |||
| #include "HashJoinRuntimeCpu.h" | |||
There was a problem hiding this comment.
The license header is missing.
| @@ -0,0 +1,71 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
The license header is missing.
|
|
||
| #include "../../../Shared/sqltypes.h" | ||
|
|
||
| namespace cpu_utils { |
There was a problem hiding this comment.
The namespace name is too common and kind of misleading. I'd say we don't use any namespace here at all.
| const int64_t max_elem, | ||
| const void* sd_inner_proxy, | ||
| const void* sd_outer_proxy) { | ||
| CHECK(sd_outer_proxy); |
There was a problem hiding this comment.
CHECK macro is not defined in this header, so header usage becomes dependent on headers including order.
| const auto sd_inner_dict_proxy = | ||
| static_cast<const StringDictionaryProxy*>(sd_inner_proxy); | ||
| const auto sd_outer_dict_proxy = | ||
| static_cast<const StringDictionaryProxy*>(sd_outer_proxy); |
There was a problem hiding this comment.
StringDictionaryProxy class is not declared in this header.
| const auto elem_str = sd_inner_dict_proxy->getString(elem); | ||
| const auto outer_id = sd_outer_dict_proxy->getIdOfString(elem_str); | ||
| if (outer_id > max_elem || outer_id < min_elem) { | ||
| return StringDictionary::INVALID_STR_ID; |
There was a problem hiding this comment.
Another reference to undeclared class.
Devjiu
force-pushed
the
dmitriim/init_hash_table_cpu_opt
branch
2 times, most recently
from
2c10582 to
aca3b31
Compare
This commit reworks `fill_hash_join_buff_bucketized_cpu` to use tbb and utilize cpu properly. Partially resolves: #574 Signed-off-by: Dmitrii Makarenko <dmitrii.makarenko@intel.com>
Devjiu
force-pushed
the
dmitriim/init_hash_table_cpu_opt
branch
from
aca3b31 to
3c9ee01
Compare
2 participants
This commit reworks
fill_hash_join_buff_bucketized_cputo use tbb and utilize cpu properly.Partially resolves: #574