Tim Armstrong has posted comments on this change. Change subject: IMPALA-3344: Simplify sorter and document/enforce invariants. ......................................................................
Patch Set 7: (7 comments) Got partway through addressing the changes, but I was thinking that the complication in SortedRunMerger stems from the need_to_return() case. If resources are attached to the batches, we just need to make sure that resources are transferred if we get a 0-row batch. We're planning to deprecate need_to_return and move to always explicitly transferring resources, so there isn't really much point to fixing this case. How about I just document that deep_copy_input_ must be true if the BatchRunSupplierFn can return batches with the need_to_return flag set. Then I can add a DCHECK to assert this. I think then the resource management will be essentially correct. http://gerrit.cloudera.org:8080/#/c/2826/7/be/src/runtime/sorted-run-merger.h File be/src/runtime/sorted-run-merger.h: Line 46: must > I don't think 'must' makes sense Done Line 48: RunBatchSupplier > Can we rename this to RunBatchSupplierFn? I find it confusing with BatchedR Done http://gerrit.cloudera.org:8080/#/c/2826/7/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 66: Runs are either "initial runs" constructed from the sorter's input or "intermediate : /// runs" constructed by merging already-sorted runs. The input rows are materialized : /// into sort tuples in the initial runs using the expressions in : /// 'sort_tuple_slot_expr_ctxs_'. > It feels like this should be woven in with the paragraph above it where we Done Line 184: return > Awkward verb conjugation throughout this sentence. Done Line 185: add > adds Done Line 974: right > right? run. oops. Line 993: TupleIterator > what do you think about having a begin() and end() static constructor? (It Done -- 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
