Michael Ho has posted comments on this change. Change subject: IMPALA-3286: Prefetching for PHJ probing. ......................................................................
Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/2959/7/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 97: malloc(sizeof(Tuple*) * num_build_tuples))) { > you don't have to do it now, but now that we have an Init() routine, it'd b Done. I still keep it as untracked memory for now. Line 853: expr_values_bytes_per_row > 0) > why do we have this check? in Init() we're DCHECK_GT zero. Is that dcheck Added an extra check for the case in which expr_values_bytes_per_row_ is 0. This if check is needed. http://gerrit.cloudera.org:8080/#/c/2959/7/be/src/exec/hash-table.h File be/src/exec/hash-table.h: Line 155: results in > evaluation to the current row of the ExprValuesCache in 'ht_ctx': the resul Done Line 186: > and the corresponding hash table buckets are prefetched before ... Done Line 230: Resets the iterators and end pointers to the start before writing > Resets the cache before starting a write phase. This is not strictly true. The content in theory is still in the cache as we didn't do a memset of the cache buffer. It's just that we reset the iterators and end pointer so the cached content cannot be iterated over anymore but they are not really gone. Line 533: Returns an iterator to the bucket matching the value of the current row evaluated : /// against the probe expressions. It's assumed that EvalAndHashProbe() has been called : /// and the evaluated values in the expression values cache of 'ht_ctx' will be used. > how this all works might still be a bit confusing to readers who don't know Done http://gerrit.cloudera.org:8080/#/c/2959/7/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 1034: ExprContext::FreeLocalAllocations(probe_expr_ctxs_); > seems like we could move this to where we re-fill the expr values cache (wh Not any particular reason other than being consistent with the pattern of free once per row batch instead of per prefetch group. -- 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: 7 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
