Jim Apple has posted comments on this change. Change subject: IMPALA-3375: Improve TopN performance with a trivial Compare object. ......................................................................
Patch Set 1: (10 comments) Result after changes and put on cdh5-trunk: http://gerrit.cloudera.org:8080/#/c/2936/ http://gerrit.cloudera.org:8080/#/c/2918/1//COMMIT_MSG Commit Message: Line 16: > Maybe mention that you renamed the operator() functions to more meaningful Done http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: Line 166: void TopNNode::InsertTupleRow(TupleRow* input_row) { > It looks like this conflicts with Skye's in-flight patch: http://gerrit.clo agreed http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/exec/topn-node.h File be/src/exec/topn-node.h: Line 76: boost::scoped_ptr<TupleRowComparator> tuple_row_less_than_; > Although now I see we have the triple-indirection here too, so I guess it's agreed. Line 103: ComparatorWrapper<TupleRowComparator> > > priority_queue_; > We shouldn't need the spaces between > > now with c++11 True, but our style guide doesn't mention this specifically. Instead, it calls out to Google's style guide, which allows either. This is also permitted in clang-format's Google mode. Emacs's C++ mode prefers the space. http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/runtime/sorted-run-merger.cc File be/src/runtime/sorted-run-merger.cc: Line 103: compare_less_than_ > The name seems a bit redundant if we rename the method - maybe just 'compar Done http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 878: while (less_than_comp_.Less( > also consider renaming to 'comparator' Done http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/service/impala-server-callbacks.cc File be/src/service/impala-server-callbacks.cc: Line 282: QueryStateRecordComparator > Did you see any performance issues with this comparator, or is this just cl Cleanup. http://gerrit.cloudera.org:8080/#/c/2918/1/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 34: constuction > construction Done Line 39: v > maybe say 'comp' instead of 'v' so it's clearer that it matches the constru Done Line 143: codegend_compare_fn_ > After reading through the patch I realise that the wrapper is kind of a gen I'm not sure there is an extra indirection in this case - the comparator in topn is already a pointer, so the wrapper is just a copy of the pointer. -- To view, visit http://gerrit.cloudera.org:8080/2918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I24755227b5bbbca6ad7c7d31d9bb8e132ca89e11 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
