Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11007 )
Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS ...................................................................... Patch Set 9: (5 comments) Lol, hopefully with some refactoring it's a little more readable - I did just put it together quickly http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream-test.cc@863 PS9, Line 863: void SimpleTupleStreamTest::TestFlushResourcesReadWrite(bool pin_stream, bool attach_on_read) { > nit: long line Done http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream-test.cc@913 PS9, Line 913: for (int j = 0; j < out_batch->num_rows(); ++j) { > style: Level 5 loop nesting seems a bit too deep to me - can you move some Done http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream-test.cc@916 PS9, Line 916: *static_cast<int*>(out_batch->GetRow(j)->GetTuple(0)->GetSlot(slot_offset))); > nit: long line Done http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream.h@395 PS9, Line 395: AttachBuffer > style: The name suggests to me that we are attaching a buffer TO the page h I used the first one. http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/11007/9/be/src/runtime/buffered-tuple-stream.cc@784 PS9, Line 784: batch->MarkFlushResources(); // TODO: remove > This seems to be called at the same conditions as line 805. If this is inte Oops, removed now. -- 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: 9 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: Sat, 28 Jul 2018 03:14:45 +0000 Gerrit-HasComments: Yes
