Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl ...................................................................... Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123 PS11, Line 123: // For now, if num_results < batch->num_rows(), we terminate returning results : // early. : if (num_results > 0 && num_results < batch->num_rows()) { > > the case in which the total number of result rows is not a multiple of ro Isn't passing different fetch size possible if the result cache is enabled ? In general, this seems to be making very big assumption about what the client will pass in. Not sure if this is exactly a good practice. I suppose my point is that we should fail stop or at least indicate some sort of failures to the clients for this known to be wrong case in the code. CHECK() is the fail-stop approach, which seems acceptable as this sink type is not anticipated to be used. An alternative would be to log this error case and return an error status to the clients. -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 11 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, 29 Jul 2019 19:48:52 +0000 Gerrit-HasComments: Yes
