Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-5498: Support for partial sorts ......................................................................
Patch Set 5: (5 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: num_rows_returned_ += row_batch->num_rows(); > Do we need to update num_rows_returned_ here? Done Line 148: } > We can't have a partial sort in a subplan, right? It's probably best to jus Done PS4, Line 149: > Let's use nullptr consistently in the new code. Done http://gerrit.cloudera.org:8080/#/c/7267/4/be/src/runtime/sorter.h File be/src/runtime/sorter.h: PS4, Line 99: ples in place. > I feel like this is breaking some of the encapsulation of the sort implemen If you prefer, I think its also reasonable to pass a 'is_partial" flag here. Both solutions are kind of weird, but with the flag we could DCHECK in various places to ensure that callers are correctly following the intention here of not mixing calls to AddBatch() and AddBatchNoSpill() (though I think it would work as expected if they did). http://gerrit.cloudera.org:8080/#/c/7267/4/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 527: if (!insertStmt.hasNoClusteredHint() && !ctx_.isSingleNodeExec()) { > Why is this limited to Kudu? We felt it was better to just do Kudu for now to keep this patch simpler and allow us to exercise it with the stress test before using it more broadly. I filed: IMPALA-5649 to track applying this to HDFS tables. -- 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: 5 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: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
