Dan Hecht has posted comments on this change. Change subject: IMPALA-3344: Simplify sorter and document/enforce invariants. ......................................................................
Patch Set 5: (14 comments) I had started on this but haven't made it through, but flushing out my comments since I see MJ gave you a review too so you can include these in the next round. http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorted-run-merger.cc File be/src/runtime/sorted-run-merger.cc: Line 70: RETURN_IF_ERROR(sorted_run_(&input_row_batch_)); don't we need to transfer ownership for any empty batches? i.e. does L64-66 need to be in this loop? But how does that work -- couldn't an empty batch be AtCapacity(), and if so don't we need to not consume more of them? Also, why is the loop needed now, whereas it was a DCHECK before? http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorted-run-merger.h File be/src/runtime/sorted-run-merger.h: Line 47: /// zero). is that now true with the loop you added? http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 48: blocks 'blocks' Line 51: block != NULL why are there NULL blocks? Line 58: two this directly contradicts the first sentence. let's reword that - the first sentence could just describe a run conceptually, and the second is the implementation. Line 59: and "that may contain"? Line 61: using a merger either delete or reference the actual class name. Otherwise, this isn't very helpful to a reader. Line 90: last final Line 106: The callee (Run) This Run Line 116: Atmost At most Line 118: all rows in output_batch will have their fixed and var-len data from : /// the same block what? doesn't var-len data live in a separate block from fixed? Line 188: tuple 'tuple' Line 189: var_len_blocks_index_ ' http://gerrit.cloudera.org:8080/#/c/2826/5/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 99: return false; return result < 0; -- 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: 5 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
