Michael Ho 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? Yes, I had hard time finding a good name. Thought about calling it 'probe_row' but it may be confusing when building the hash table. Passing &row directly should work too but I want to avoid overwriting reference to row in case we need it after calling Equals() such as the case where we are storing tuple directly in the hash table. I keep it on the stack as a temp so it can be overwritten with NULL to indicate that lazy evaluation has happened. Line 58: GetRow(bucket, ht_ctx->row_))) { > Logically, I think it makes more sense to do the lazy eval in this function We still have to differentiate between the build and probe exprs here (as we are mirroring what EvalAndHashBuild() and EvalAndHashProbe() are doing) and there is no easy to tell as far as I know. Doing in Equals() makes it easier as we replace EvalRow() in place with either the codegen'ed version of EvalBuildRow() or EvalProbeRow(). 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' Unfortunately, this is called by hash-table-test.cc for some reasons. I can look into converting or removing those callers if possible. Line 135: NULL > does this work correct? Equals() will end up getting a pointer to pointer t Not sure what you meant ? Doesn't Equals() check *expr_values_row ? *expr_values_row == NULL in this case so evaluation will be skipped. I will rename the argument in Equals() to expr_values_row_ptr instead to avoid confusion. -- 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
