[Impala-ASF-CR] WIP IMPALA-9434: Implement Robin Hood Hash Table.

["David Rorke (Code Review)" <ge...@cloudera.org>]

David Rorke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15511 )

Change subject: WIP IMPALA-9434: Implement Robin Hood Hash Table.
......................................................................


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15511/9/be/src/exec/hash-table.inline.h
File be/src/exec/hash-table.inline.h:

http://gerrit.cloudera.org:8080/#/c/15511/9/be/src/exec/hash-table.inline.h@92
PS9, Line 92: return bucket_idx
I understand this is the short-cut case for insert, and returning the index of a non-empty bucket might work here when called during insert, but it may not follow the contract described for the Probe() return value (see the comment in hash-table.h).  Calls to Probe during lookup won't necessarily check the value of "found" and might not expect a return that's not one of (a) a match, (b) an empty bucket, or (c) BUCKET_NOT FOUND.  For example not clear whether call from FindBuildRowBucket would expect a non-empty bucket as an indication of probe failure.

I'm not sure what the best solution is given the need to return a swap target bucket somehow in the insert case, but it seems like we should do something to make the contract for the return value clearer and less overloaded.


http://gerrit.cloudera.org:8080/#/c/15511/9/be/src/exec/hash-table.inline.h@132
PS9, Line 132: my_distance
Should my_distance be set to the BucketDistance of the new bucket in the case where we swap?



-- 
To view, visit http://gerrit.cloudera.org:8080/15511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28eeccd7f9ccae39e31972391f971901bcbfe986
Gerrit-Change-Number: 15511
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Apr 2020 01:41:53 +0000
Gerrit-HasComments: Yes