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 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/buffered-tuple-stream-test.cc@323 PS7, Line 323: "-1" nit: use the name of the test instead. Same below. http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.h File be/src/runtime/spillable-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.h@100 PS7, Line 100: BufferedTupleStream* batch_queue_; Shouldn't this use unique_ptr ? May be I am missing something but as the code stands now, aren't we leaking 'batch_queue_' ? http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.cc File be/src/runtime/spillable-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/runtime/spillable-row-batch-queue.cc@53 PS7, Line 53: batch_queue_ = new Use unique_ptr http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@907 PS7, Line 907: query_options->max_pinned_result_spooling_memory nits: code seems more legible if we factor out these two options into local variables: int64_t max_pinned_mem = ... int64_t max_unpinned_mem = ... http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@921 PS7, Line 921: if (query_options->max_pinned_result_spooling_memory == 0 : && query_options->max_unpinned_result_spooling_memory != 0) May be I mis-read it but isn't this the same check as the one on line 907 ? http://gerrit.cloudera.org:8080/#/c/14039/7/be/src/service/query-options.cc@928 PS7, Line 928: if (query_options->max_unpinned_result_spooling_memory != 0 : && query_options->max_unpinned_result_spooling_memory : < query_options->max_pinned_result_spooling_memory) Same question: isn't this the same as the one at line 912 ? http://gerrit.cloudera.org:8080/#/c/14039/7/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14039/7/common/thrift/ImpalaService.thrift@419 PS7, Line 419: MAX_PINNED_RESULT_SPOOLING_MEMORY = 86 Also, please specify the behavior when setting this to 0. Same for any value < 0. -- 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: 7 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: Tue, 20 Aug 2019 00:49:51 +0000 Gerrit-HasComments: Yes
