Csaba Ringhofer 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) The new test seems mind-bending this late at night. I will give it another run on Monday. I have some nits and style comments till then. 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 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 of the inner loops into separate functions? For example the verifying logic from line 908 to 918 seems a good candidate to me. This is definitely subjective, so feel free to keep it as it is. 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 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 here. Some names that came to my mind are "AttachBufferToBatch", "ExtractBuffer" or "TakeBuffer". 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 intentional, can you explain why? -- 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: Fri, 27 Jul 2018 23:03:35 +0000 Gerrit-HasComments: Yes
