Tim Armstrong has posted comments on this change. Change subject: IMPALA-5498: Support for partial sorts ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/7267/4/be/src/exec/partial-sort-node.cc File be/src/exec/partial-sort-node.cc: Line 111: *eos = true; Do we need to update num_rows_returned_ here? Line 148: We can't have a partial sort in a subplan, right? It's probably best to just put a DCHECK(false) here since there's no way to test the code. PS4, Line 149: :Res Let's use nullptr consistently in the new code. http://gerrit.cloudera.org:8080/#/c/7267/3/be/src/runtime/sorter.h File be/src/runtime/sorter.h: Line 96: Sorter(const TupleRowComparator& compare_less_than, > This will be included in a follow up patch. WFM. If you want to wait for https://gerrit.cloudera.org/#/c/5801/ to land there will be a way to directly set a maximum on bytes of buffers in the planner, which would accomplish the same thing. http://gerrit.cloudera.org:8080/#/c/7267/4/be/src/runtime/sorter.h File be/src/runtime/sorter.h: PS4, Line 99: RuntimeProfile* profile, Run I feel like this is breaking some of the encapsulation of the sort implementation. This detail leaks out into the planner anyway so I guess it's not really encapsulated. How about we just document more clearly what value is expected. The class comment talks about the number of blocks per run, so it could be extended to talk about the minimum requirement for spilling and non-spilling sorts. http://gerrit.cloudera.org:8080/#/c/7267/3/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: PS3, Line 349: highest/lowest > My problem with that wording is that it could be interpreted as "take the f WFM -- To view, visit http://gerrit.cloudera.org:8080/7267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieec2a15a0cc5240b1c13682067ab64670d1e0a38 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
