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 <riza.sumi...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 02 Apr 2020 01:41:53 +0000 Gerrit-HasComments: Yes
