Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10449 )
Change subject: IMPALA-7011: Simplify PlanRootSink control logic ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/10449/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10449/3//COMMIT_MSG@7 PS3, Line 7: IMPALA_7011 > nit: IMPALA-7011 Done http://gerrit.cloudera.org:8080/#/c/10449/3//COMMIT_MSG@20 PS3, Line 20: control > nit: controlled Done http://gerrit.cloudera.org:8080/#/c/10449/3//COMMIT_MSG@24 PS3, Line 24: to > nit: extra word? Done http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.h File be/src/exec/plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.h@57 PS3, Line 57: respecively > respectively Done http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.h@102 PS3, Line 102: Canecl > Cancel Done http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.h@106 PS3, Line 106: /// State of the sender: > Producer instead of sender? Seems inconsistent with above. Did the opposite, only because the code variables use sender more (so changed the comments from producer to sender above). http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.h@107 PS3, Line 107: MORE_ROWS > nit: how about ROWS_PENDING Done http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.cc File be/src/exec/plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.cc@a76 PS3, Line 76: > just curious, was this dead code all along or is there ever a possibility o I didn't look back at git history, but like you said, it's clearly a dead conditional. It may have been possible to pass batch == nullptr before the FragmentInstanceState refactor. Okay, looked at git history, and this was a do-while loop (so the dereference wasn't there. But still, ValidateCollectionSlots() dereferences. Plus, it doesn't make sense to do the check after the Wait() anyway. So, I'm not really sure why it was written. http://gerrit.cloudera.org:8080/#/c/10449/3/be/src/exec/plan-root-sink.cc@116 PS3, Line 116: enounters > encounters Done -- To view, visit http://gerrit.cloudera.org:8080/10449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc75617a253fd43a6122baa4b4dc7aeb1dbe633f Gerrit-Change-Number: 10449 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 18 May 2018 20:19:40 +0000 Gerrit-HasComments: Yes
