Tim Armstrong has posted comments on this change. Change subject: Impala-3286: Prefetching For Phj Probing. ......................................................................
Patch Set 4: (8 comments) I think we're getting close. Did we do any benchmarking on very small build-sides, e.g. 10s of rows, to confirm there wasn't an impact there? http://gerrit.cloudera.org:8080/#/c/2959/4/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 138: /// TODO: figure out which hash function to use. We need to generate uncorrelated This comment seems outdated. Let's remove it. Line 242: expr_values_bytes_per_row_ Maybe we should DCHECK that this is > 0? http://gerrit.cloudera.org:8080/#/c/2959/4/be/src/exec/hash-table.h File be/src/exec/hash-table.h: Line 266: void IR_ALWAYS_INLINE SetAtEnd(); Where is this defined? Line 433: /// Used by PHJ to store results of batch evaluations of rows. The PHJ references will probably get stale. Maybe just say "Use to store results of batched evaluation of rows". http://gerrit.cloudera.org:8080/#/c/2959/4/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 378: not : // empty 'partially full' instead of 'not empty' seems clearer. Line 380: works work instead of works. Line 510: hash_tbl_->PrefetchBucket<false>(expr_vals_cache->ExprValuesHash()); Tabs? http://gerrit.cloudera.org:8080/#/c/2959/4/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 915: ExprContext::FreeLocalAllocations(build_expr_ctxs_); Would it make more sense to do this after returning from ProcessProbeBatch()? E.g. after we commit the rows to the batch. It seems like that's the point when it's safe to free the local allocations. -- 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: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
