Tim Armstrong has posted comments on this change. Change subject: IMPALA-3332: Free local allocations in sorter. ......................................................................
Patch Set 1: (7 comments) It would be good to run a couple of the primitive_orderby targeted-perf queries just to make sure there aren't any surprises. I feel like this shouldn't make much of a difference, but let's be sure. http://gerrit.cloudera.org:8080/#/c/3109/1/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 894: bool is_less = comparator_.Less(lhs, rhs); This might be more concise if we call Less() after doing the comparison_count_ check. Line 897: state_->batch_size() Maybe cache this, or just use a sensible constant like 1024? Maybe it doesn't matter too much, but the Partition() loop is the main hot loop for the sorter. Maybe make it UNLIKELY() too? Line 936: if (UNLIKELY(state_->is_cancelled())) return Status::CANCELLED; We insertion sort max 16 elements, so I think we can elide these checks. Line 948: while (true) { This loop can go on a long time - it does a pass over the entire buffered input, potentially many GB. We may want to add status checks in the loop - or convince ourselves that it's ok to elide them. Line 953: while (Less(temp_tuple_row_, reinterpret_cast<TupleRow*>(&last.current_tuple_))) { It's nice that this works out more readable. http://gerrit.cloudera.org:8080/#/c/3109/1/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 136: Extra line. http://gerrit.cloudera.org:8080/#/c/3109/1/testdata/workloads/functional-query/queries/QueryTest/sort.test File testdata/workloads/functional-query/queries/QueryTest/sort.test: Line 4112: # periodically. disable_outermost_topn=1 forces sort node to be used. There are a number of tests in spilling.test that have mem_limits commented out because of this bug. We should enable them. I think this test here might be redundant with those. -- To view, visit http://gerrit.cloudera.org:8080/3109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id658243d4ffa9dcdbf4e867db0bb3e8d161af086 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
