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 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node-ir.cc File be/src/exec/select-node-ir.cc: http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node-ir.cc@28 PS7, Line 28: limit_ This could return extra rows. E.g. if the limit for the node is 1025, we returned a batch of 1024 rows and are now processing another input batch of 1024 rows, then this would return another 1024 rows instead of only 1 more. It seems like we might be missing some test coverage here of select nodes with limits (it may also require a single node plan if there's an exchange inserted otherwise). I think the old ReachedLimit() check plus FOREACH_ROW would work. http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h File be/src/exec/select-node.h: http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@70 PS6, Line 70: Status CodegenCopyRows(RuntimeState* state); > Good to know. Apparently missed this change while I was gone. Yeah, there were actually two LLVM changes that might be of interest to you: https://gerrit.cloudera.org/#/q/status:merged+project:Impala-ASF+branch:master+topic:llvm-upgrade http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc@91 PS7, Line 91: DCHECK(*eos == false); This isn't safe - the GetNext() API doesn't require that the caller initializes the output parameter. (IMO eos should be part of the ExecNode state rather than something transiently set as an output parameter, but that's a different discussion!) http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc@112 PS7, Line 112: nit: remove extra vertical whitespace -- 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: 7 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: Tue, 10 Oct 2017 22:38:56 +0000 Gerrit-HasComments: Yes
