Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14129 )
Change subject: IMPALA-8819: BufferedPlanRootSink should handle non-default fetch sizes ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@85 PS2, Line 85: > QueryState::DEFAULT_BATCH_SIZE ? Done http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@136 PS2, Line 136: atch. > nit: unnecessary for unique_ptr Done http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@144 PS2, Line 144: /// the logical queue of RowBatches and thus includes any RowBatch that > This needs to be called with 'lock_' held right ? Yeah, updated the code comments above. http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.h@145 PS2, Line 145: /// 'current_batch' p > bool IsQueueEmpty const { Done http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc@143 PS2, Line 143: num_results = min(num_results, MAX_FETCH_SIZE); > Thanks for doing the experiment. 10x BATCH_SIZE seems like a reasonable sta Done http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc@152 PS2, Line 152: while (!*eos && num_rows_read < num_rows_to_read) { > Don't need to solve in this PS, but I think we could solve IMPALA-7312 with Sounds good. Will work on IMPALA-7312, but in a different PS. http://gerrit.cloudera.org:8080/#/c/14129/2/be/src/exec/buffered-plan-root-sink.cc@162 PS2, Line 162: eos and then return. The queu > if (!current_batch_) Done http://gerrit.cloudera.org:8080/#/c/14129/2/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/14129/2/tests/query_test/test_result_spooling.py@264 PS2, Line 264: > Given the assert at line 268, why not just do: Done -- To view, visit http://gerrit.cloudera.org:8080/14129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8dd4b397ab6457a4f85e635f239b2c67130fcce4 Gerrit-Change-Number: 14129 Gerrit-PatchSet: 3 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: Mon, 26 Aug 2019 01:09:45 +0000 Gerrit-HasComments: Yes
