Dan Hecht has posted comments on this change. Change subject: IMPALA-1346/1590: fix sorter buffer mgmt when spilling ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/2908/5//COMMIT_MSG Commit Message: Line 7: IMPALA-1346/1590: fix sorter buffer mgmt when spilling IMPALA-2344 too? http://gerrit.cloudera.org:8080/#/c/2908/5/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 1447 why does this go away? Line 1481: blocks_per_run how about renaming this pinned_blocks_per_run? Line 1484: blocks one or two blocks .. (when I first read the comment i wondered why we claimed that runs blocks were all pinned) Line 1497: and may in case we .. to clarify that we might not use this output run but don't know yet. Line 1519: // Should have errored out before this point if we had only two runs. not sure what this comment is saying. is it referring to this dcheck in some way? Line 1530: DCHECK_GT(max_num_runs, 1); DCHECK_GE(sorted_runs_.size(), 2) or is that not the case? i guess from MergeIntermediateRuns we can sometimes end up with the final run "merging" a single sorted run? Line 1547: set up a merge have at least two runs ready to merge Line 1551: if (merging_runs_.size() >= 2) break; okay to ignore but I think it's easier to read as: if (merging_runs_.size() < 2) { return status; } // Merge the runs we were able to prepare. break; http://gerrit.cloudera.org:8080/#/c/2908/5/be/src/runtime/sorter.h File be/src/runtime/sorter.h: Line 130: least two runs is that true? what if on input sorted_runs_.size() == 1 (see my question in .cc file) -- To view, visit http://gerrit.cloudera.org:8080/2908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idfe55cc13c7f2b54cba1d05ade44cbcf6bb573c0 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
