Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8196 )
Change subject: IMPALA-4236: Codegen CopyRows() for select nodes ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/8196/4/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8196/4/be/src/exec/select-node.cc@113 PS4, Line 113: copy_rows_result; I think it would still be better to have a name that makes the below control flow more understandable, if we can come up with one. It is unnecessarily confusing right now. After looking at this again with the benefit of some time, I think having CopyRows() returning a bool is unnecessarily confusing because it's telling the caller about the current state in a non-obvious way, yet the caller can check the state directly. http://gerrit.cloudera.org:8080/#/c/8196/4/be/src/exec/select-node.cc@120 PS4, Line 120: if (copy_rows_result) { I think there's an equivalent but simpler way of expressing the control flow along the lines of: *eos = ReachedLimit() || (child_row_idx_ == child_row_batch->num_row() && child_eos_); if (*eos) { ... do the eos things ... return Status::OK(); } if (row_batch->AtCapacity()) return Status::OK(); I don't think your change made this more confusing but I think it would be good to fix it so that the control flow is more obviously correct. -- To view, visit http://gerrit.cloudera.org:8080/8196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0d496d004418468e16b6f564f90f45ebbf87c1e Gerrit-Change-Number: 8196 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 04 Oct 2017 18:58:38 +0000 Gerrit-HasComments: Yes
