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/1/be/src/exec/partial-sort-node.cc File be/src/exec/partial-sort-node.cc: Line 104: sorter_->Reset(); > To be clear - what you're saying is that Open() will want to allocate memor I wouldn't expect incorrect results, but it could result in queries failing with OOM when they shouldn't. http://gerrit.cloudera.org:8080/#/c/7267/3/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 1384: int min_buffers_required = MIN_BUFFERS_PER_MERGE; Need to update this calculation for partial sorts. 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, Wasn't the plan to apply the partition sort memory limit to the sorter itself? http://gerrit.cloudera.org:8080/#/c/7267/3/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: PS3, Line 349: highest/lowest Should this be "first" since the order is specified elsewhere. Maybe: "Return the first N elements in sorted order"? http://gerrit.cloudera.org:8080/#/c/7267/3/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: Line 58: private final long PARTIAL_SORT_MEM_LIMIT = BackendConfig.INSTANCE.getReadSize() * 100; It might be clearer to specify this in bytes, then just make sure that its >= 2 * read_size. Although we really don't expect people to change read_size. Line 286: resourceProfile_ = new ResourceProfile( This looks good to me. -- 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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
