Dan Hecht has posted comments on this change. Change subject: IMPALA-3286: Software prefetching for hash table build. ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/2896/4/be/src/exec/hash-table.inline.h File be/src/exec/hash-table.inline.h: Line 52: expr_values_row this name doesn't really make sense. Why not just pass in '&row' below? Line 58: GetRow(bucket, ht_ctx->row_))) { Logically, I think it makes more sense to do the lazy eval in this function. That way, Equals() doesn't have to know about the 'did we already eval' state. It might also make it simpler once you want to do lazy eval on the Probe() side since the C++ Equals() code would have to know whether it is build or probe exprs. But here you know it should be the build exprs. And you can still replace EvalBuildRow() in codegen (it should just work and find 2 occurrence). I think this would also fix the problem noted below in FindProbeRow() and FindBuildRowBucket(). I'll hold off on reviewing the rest of the patch until we decide this point. Line 112: inline bool HashTable::Insert(HashTableCtx* ht_ctx, TupleRow* row, uint32_t hash) { is this function called anywhere else besides line 102? If not, I think it'd be simpler to get rid of it and just rewrite Insert() as below. It'll make documenting easier with the row/tuple replacement. HtData* htdata = InsertInternal(ht_ctx, row, hash); if (LIKELY(htdata != NULL) { if (stores_tuples_) { htdata->tuple = row-GetTuple(0); } else { htdata->idx = idx; } return true; } return false; Line 135: NULL does this work correct? Equals() will end up getting a pointer to pointer to NULL, so it'll still want to evaluate. -- To view, visit http://gerrit.cloudera.org:8080/2896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib85e7fc162ad25c849b9e716b629e226697cd940 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
