Tim Armstrong has posted comments on this change. Change subject: IMPALA-3168: replace HashTable parameters with constants ......................................................................
Patch Set 5: (15 comments) http://gerrit.cloudera.org:8080/#/c/3088/5/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 473: /// value 'replacement'. > Would be good to comment on the return value as this is pretty far away fro Done http://gerrit.cloudera.org:8080/#/c/3088/5/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 88: int num_build_tuples, > this shouldn't still be here Yep, missed it in the code churn here. Line 121: num_build_tuples > delete Done Line 1160: int num_build_tuples, > Is this used at all in this function ? See line 1170 http://gerrit.cloudera.org:8080/#/c/3088/5/be/src/exec/hash-table.h File be/src/exec/hash-table.h: Line 118: /// - num_build_tuples: number of tuples in the row stored in the table > delete Done Line 128: num_build_tuples > delete Done Line 436: > maybe comment about how these are replaced with constants like the other co Done Line 463: num_build_tuples_ > not sure this comment still makes sense Done. Reverted the comment change. Line 723: friend class PartitionedAggregationNode; > still here. did you forget to squash a change? Done Line 828: > extra blank line? Removed. http://gerrit.cloudera.org:8080/#/c/3088/5/be/src/exec/hash-table.inline.h File be/src/exec/hash-table.inline.h: Line 191: *node = NULL; > as mentioned in other patch, consider adding DCHECK(*node == NULL) to top o I ended up switching approaches slightly so that *node is always set to NULL when stores_duplicates() is false. Line 242: inline > i don't think we usually put 'inline' when IR_ALWAY_INLINE is there The 'inline' is actually needed to force the right linkage and avoid multiple definitions when linking, so in this case it isn't redundant. I realised that I had IR_ALWAYS_INLINE in non-standard place (before instead of after return type), so I fixed that. Line 246: iterators Fixed my own typo. http://gerrit.cloudera.org:8080/#/c/3088/5/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 458: stores_duplicates, > Just curious why we cannot just put true here ? I thought the existing patt Yeah that would be more consistent. Changed. I think sometimes we'd be better off defining enums for some of these kind of parameters but not going to do that in this patch. Line 1858: GE > why GE rather than EQ the actual count? not worth the pain of maintaining? Yeah that was my thought - it seemed like the exact counts could easily be affected by minor changes to HashTable. -- To view, visit http://gerrit.cloudera.org:8080/3088 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I483a19662c90ca54bc21d60fd6ba97dbed93eaef Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
