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: Software prefetching for hash table build.
Date Mon, 02 May 2016 18:46:33 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@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