Tim Armstrong has posted comments on this change. Change subject: IMPALA-3375: Improve TopN performance with a trivial Compare object. ......................................................................
Patch Set 1: (10 comments) Looking pretty good, mainly minor comments and curiousity about whether we can make it better. 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 names. 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.cloudera.org:8080/#/c/1901/ but I think it should be trivial to resolve. 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 not just with the new comparator wrapper. Line 103: ComparatorWrapper<TupleRowComparator> > > priority_queue_; We shouldn't need the spaces between > > now with c++11 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 'comparator_'? 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' 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 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 Line 39: v maybe say 'comp' instead of 'v' so it's clearer that it matches the constructor argument. Line 143: codegend_compare_fn_ > We introduced a pointer indirection here in a different patch, so the wrapp After reading through the patch I realise that the wrapper is kind of a generic adapter. I think it's worth seeing if the indirection comes with a performance cost - it might be a win to avoid that. -- 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: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
