Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-5520: TopN node periodically reclaims old allocations ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/7400/3/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: PS3, Line 244: tuple_row_comparator_wrapper_ > I think something like ComparatorWrapper(*tuple_row_less_than_) will work. Please refer to related comment in topn-node.h PS3, Line 259: NULL > It would be best to pass in the RuntimeState* - that enables better logging Done PS3, Line 271: NULL > Same here. Done http://gerrit.cloudera.org:8080/#/c/7400/3/be/src/exec/topn-node.h File be/src/exec/topn-node.h: Line 36: #define PUSH_HEAP(v, comp, elem) v.push_back(elem); \ > It's probably best to just make it an inline function, it doesn't seem like Done Line 100: boost::scoped_ptr<ComparatorWrapper<TupleRowComparator>> tuple_row_comparator_wrapper_; > We don't need to store this additional state, since all the state is in tup True, but every time POP_HEAP or PUSH_HEAP is used, it will incur the cost of creating a new wrapper object,which will esp be a problem in InsertTupleRow method PS3, Line 134: vector > Also mention that it's updated with push_heap()/pop_heap(). Done -- To view, visit http://gerrit.cloudera.org:8080/7400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I968f57f0ff2905bd581908bc5c5ee486b31e6aa8 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
