impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3286: Prefetching for PHJ probing.
Date Sun, 15 May 2016 03:23:10 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message