impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) Impala-3286: Prefetching For Phj Probing.
Date Thu, 12 May 2016 22:26:23 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@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