Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4883: Union Codegen ......................................................................
Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/6459/8/be/src/codegen/gen_ir_descriptions.py File be/src/codegen/gen_ir_descriptions.py: PS8, Line 183: ["UNION_MATERIALIZE_BATCH", : "_ZN6impala9UnionNode16MaterializeBatchEPNS_8RowBatchEPPh"], > nit: please consider putting it in a different entry so as not to break up Done http://gerrit.cloudera.org:8080/#/c/6459/8/be/src/exec/union-node.cc File be/src/exec/union-node.cc: PS8, Line 223: codegend_union_materialize_batch_fns_.data() > Why not codegend_union_materialize_batch_fns_[child_idx_] ? Same below. Done Line 290: int num_rows_before = row_batch->num_rows(); > So, in that case, is it true that the rows in the non-empty batch are alrea Yes, if a batch is not empty, the rows in it should already be counted towards num_rows_returned_ at this point. If on line 295, some rows get added the the batch, those are not added to num_rows_returned_ yet. num_rows_returned gets updated on line 306. In other words, num_rows_returned gets updated only in one place: line 306. PS8, Line 304: num_rows_added > I probably misunderstood something here. Can you please explain why we need Turns out this formula was wrong. I added a test case added a test case (which fails on Patch Set 8 and passes on the latest patch set). -- To view, visit http://gerrit.cloudera.org:8080/6459 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
