Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11007 )
Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in aggs and joins ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@801 PS3, Line 801: true > Shouldn't we pass attach_on_read here? Yeah. It looks like I pushed out a stale version of the patchset missing some changes I made to this test. http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream-test.cc@805 PS3, Line 805: ASSERT_OK(stream.GetNext(out_batch, &eos)); > Can you add an assert to check that out_batch->num_buffers() == 0 before Ge Done. It is guaranteed by the out_batch->Reset() at the bottom of the loop but seems worth asserting. http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/11007/3/be/src/runtime/buffered-tuple-stream.h@184 PS3, Line 184: /// caller is responsible for managing the lifetime of those buffers. : /// 3. If the stream is unpinned and not in attach on read mode, then the batch returned : /// fro > nit: is there a reason behind indenting these line with +1 space? Nope, I'll make it consistent with above. -- To view, visit http://gerrit.cloudera.org:8080/11007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5 Gerrit-Change-Number: 11007 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 24 Jul 2018 16:50:34 +0000 Gerrit-HasComments: Yes
