Tim Armstrong has posted comments on this change. Change subject: IMPALA-3344: Simplify sorter and document/enforce invariants. ......................................................................
Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/2826/7/be/src/runtime/sorted-run-merger.cc File be/src/runtime/sorted-run-merger.cc: Line 33: BatchedRowSupplier > A few high level things: I couldn't think of anything much clearer. Done. 2) See my earlier comment - I think the right thing to do here is to avoid handling need_to_return() and simplify this. I think the three general alternatives here are a) copy the rows so they don't reference anything passed up from the sorter, b) do the complicated TryAdvance() handshake so that we can free resources before moving to the next row or c) use double the resources while transitioning between blocks. It doesn't seem like the copying is a major performance bottleneck right now, so it's probably the best approach until it shows up on profiles. Line 55: exhausted > exhausted, Done Line 57: resouce > resources Done Line 59: Next > Per my larger comment at the top, how about: I think 1) and 2) aren't relevant any more since I removed handling of the need_to_return case. I did rename Next() to Advance(). 3) is done. Line 86: there is at least one row remaining in the batch. > ...at least one row (including current_row()) remaining in the current batc Done Line 87: before 'done' is returned. > before Next() returns with 'done' set to true Done http://gerrit.cloudera.org:8080/#/c/2826/7/be/src/runtime/sorted-run-merger.h File be/src/runtime/sorted-run-merger.h: Line 69: If 'deep_copy_input_' is false > If true, this should always be able to advance, right? Can you maybe add th With the other changes this got simplier. Line 78: AdvanceMinRow > TryAdvanceMinRow? See above. -- To view, visit http://gerrit.cloudera.org:8080/2826 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c619e81fd1b8ac50e257172c8bce101a112b52a Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
