Michael Ho has posted comments on this change. Change subject: IMPALA-3286: Prefetching for PHJ probing. ......................................................................
Patch Set 11: (15 comments) http://gerrit.cloudera.org:8080/#/c/2959/11/be/src/exec/hash-table-test.cc File be/src/exec/hash-table-test.cc: Line 185: runtime_state_->InitMemTrackers(TUniqueId(), NULL, -1); > Let's just do this in CreateQueryState(). Done Line 241: runtime_state_->query_mem_tracker() > It would be better to use the test mem_tracker_ we set up. Done http://gerrit.cloudera.org:8080/#/c/2959/11/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 125: // TODO: use tr1::array? > We can delete this TODO, I don't think it's very helpful, especially since Done Line 131: inline > This probably doesn't need to be inline, does it? Yes, kept it when moving it from the header file to here. Probably not worth it. http://gerrit.cloudera.org:8080/#/c/2959/11/be/src/exec/hash-table.h File be/src/exec/hash-table.h: Line 126: /// Allocate various buffers for storing expression evaluation results, hash values, > We should have a HashTableCtx::Create() function that allocates and initial Good idea. Done. Line 128: Status Init(RuntimeState* state, int num_build_tuples, MemTracker* tracker); > Also mention what 'tracker' is (it seems at least as important to document Added comments in the constructor. Line 130: /// Call to cleanup any resources. > Mention that 'tracker' is the same 'tracker' passed to Init(). Done Line 226: resourcs > resources. Done Line 346: Bitmap null_bitmap_; > I feel like it would be simpler to just do an array of uint8_t to be consis IMHO, higher memory density means lower cache footprint and helps reduce the pressure of L1 cache so I think there is some benefit for using bitmap here as a larger boolean array may end up evicting other hot data from the relatively small L1 cache. I actually think we should convert the expr_values_null_array_ to bitmap too given its potential size but I will not do so in this change as it bitmap interface may need to be changed slightly. http://gerrit.cloudera.org:8080/#/c/2959/11/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 336: batch_size > cache_size Renamed to prefetch_size to be consistent with InsertBatch() below. Line 378: has_probe_rows This fixes IMPALA-3510. Line 476: row_idices > typo: row_indices Done Line 477: cur_row > The logic around cur_row is a little hard to follow, since it is the loop v Done http://gerrit.cloudera.org:8080/#/c/2959/11/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 678: probe_exp_ctxs_ > Typoe: 'expr_ctxs_' Done Line 1005: > Extra space Done -- To view, visit http://gerrit.cloudera.org:8080/2959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib42b93d99d09c833571e39d20d58c11ef73f3cc0 Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
