Michael Ho 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) LGTM. Mostly minor comments for the first pass. Will do a second pass again today. 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 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 ? 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. 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 ? 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 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 ? 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: // /// 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& 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 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()); -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 15 Aug 2019 18:47:25 +0000 Gerrit-HasComments: Yes
