impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-3168: replace HashTable parameters with constants
Date Tue, 17 May 2016 01:46:44 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3168: replace HashTable parameters with constants

Patch Set 5:


Please do a patch that addresses the comment before a rebase so I can just look at the diff,
then rebase patch is fine after that one.
File be/src/exec/

Line 88:  int num_build_tuples,
this shouldn't still be here

Line 121: num_build_tuples
File be/src/exec/hash-table.h:

Line 118:   ///  - num_build_tuples: number of tuples in the row stored in the table

Line 128: num_build_tuples

Line 436: 
maybe comment about how these are replaced with constants like the other comment

Line 463: num_build_tuples_
not sure this comment still makes sense

Line 723:     friend class PartitionedAggregationNode;
still here. did you forget to squash a change?
File be/src/exec/hash-table.inline.h:

Line 191:   *node = NULL;
as mentioned in other patch, consider adding DCHECK(*node == NULL) to top of this function
and removing this line, to be consistent with the return case for !stores_duplicates(). otherwise,
the reader wonders whether a *node = NULL is missing in that case.

Line 242: inline
i don't think we usually put 'inline' when IR_ALWAY_INLINE is there
File be/src/exec/

Line 1858: GE
why GE rather than EQ the actual count? not worth the pain of maintaining? (fine with me to
leave it this way if that's why).

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I483a19662c90ca54bc21d60fd6ba97dbed93eaef
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message