Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14039 )
Change subject: IMPALA-8818: Replace deque with spillable queue in BufferedPRS ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.h@49 PS4, Line 49: Intializes > typo Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/buffered-plan-root-sink.cc@19 PS4, Line 19: #include "runtime/deque-row-batch-queue.h" > Can be deleted ? Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/exec/exec-node.cc@373 PS4, Line 373: node_type_name + " (id=" + std::to_string(id_) + ")" > Use substitute for better legibility. Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.h@223 PS4, Line 223: const std::string > Did you mean to pass-by-value so the compiler will do copy-elison ? Changed to pass-by-reference to avoid an additional copy, but I think there is still a copy happening during the assignment: caller_label_ = caller_label Not sure if there is a way to avoid that? http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/buffered-tuple-stream.cc@212 PS4, Line 212: string caller_label > Doesn't match the signature in header Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/deque-row-batch-queue.h File be/src/runtime/deque-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/deque-row-batch-queue.h@33 PS4, Line 33: class DequeRowBatchQueue { > Can this class and files be deleted after this patch ? Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/sorter.h@210 PS4, Line 210: // > /// Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h File be/src/runtime/spillable-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h@57 PS4, Line 57: std::string &name > nit: std::string& Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.h@107 PS4, Line 107: std::string &n > nit: std::string& name Done http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.cc File be/src/runtime/spillable-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/14039/4/be/src/runtime/spillable-row-batch-queue.cc@101 PS4, Line 101: DCHECK((eos && IsEmpty()) || (!eos && !IsEmpty())); > DCHECK_EQ(eos, IsEmpty()); Done -- To view, visit http://gerrit.cloudera.org:8080/14039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10f9e72374cdf9501c0e5e2c5b39c13688ae65a9 Gerrit-Change-Number: 14039 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 19 Aug 2019 18:54:45 +0000 Gerrit-HasComments: Yes
