Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17195 )
Change subject: IMPALA-10584: Defer advancing read page if stream only has 2 pages. ...................................................................... Patch Set 6: (8 comments) Patch set 5 change the condition on when to NOT advance the read page. Read page will not be advanced in UnpinStream if read page is attached to output RowBatch and there are only 2 pages in the stream. http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1271 PS5, Line 1271: ASSERT_OK(stream.UnpinStream(BufferedTupleStream::UNPIN_ALL_EXCEPT_CURRENT)); > Not related to this patch, but I'm confused how do we verify we have advanc I think this verify the read page has been advanced by not hitting DCHECK at ExpectedPinCount(). http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1300 PS5, Line 1300: // Test that UnpinStream defer advancing the read page when all rows from the read page > Could you add some comments above this? e.g. Done http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1333 PS5, Line 1333: ASSERT_TRUE(read_batch.num_rows() < num_rows); > Could you add a comment here that we are adding rows without releasing the Done http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1334 PS5, Line 1334: ASSERT_TRUE(!eos); > Could you leave a comment about why do we run this twice? I guess we want t Done http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1354 PS5, Line 1354: // Retry inserting this row by decreasing the index. > Could you add ASSERT_FALSE(stream.is_pinned()) at the end? Done http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream-test.cc@1356 PS5, Line 1356: // even if we're not immediately cleaning up the read_batch. > Do we need read_batch.Reset() as well? Done http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream.cc@728 PS5, Line 728: // defer advancing the read page until the next GetNext() call by the reader : // (see IMPALA-10584). : defer_advancing_read_page = true; : } : } : : if > Ok, after some investigation, I'm not confident I can implement solution 2) Marking this as Done. Lets continue this discussion in patch set 5. http://gerrit.cloudera.org:8080/#/c/17195/5/be/src/runtime/buffered-tuple-stream.cc@747 PS5, Line 747: > May DCHECK() on this assertion here. Done -- To view, visit http://gerrit.cloudera.org:8080/17195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16137b6e423f190f60c3115a06ccd0f77e9f585a Gerrit-Change-Number: 17195 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 14 Apr 2021 22:22:31 +0000 Gerrit-HasComments: Yes
