Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2784: codegen PartitionedHashJoinNode::Partition::BuildHashTable() ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/2113/5/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 294: void* e = filter_ctx.expr->GetValue(row); > leave todos to codegen the expr evaluation and hash value computation as we This was moved into ProcessBuildBatch(). I'll add the TODO there. http://gerrit.cloudera.org:8080/#/c/2113/5/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 241: AddCodegenExecOption(insert_codegen_enabled, codegen_status, "Insert"); > is there really a chance of this being disabled but "build side" being enab It's hard to reason about what conditions would cause one or both to fail, and it could change in the future. I think it's safer and easier to just include this, since it's a separate codegen function. "Hash table construction" sounds good to me. Line 1842: Function* insert_batch_fn = codegen->GetFunction(IRFunction::PHJ_INSERT_BATCH, true); > avoid single-line blocks I moved this line to L1839, and combined a few other small blocks. I think L1856 should remain by itself though since it doesn't belong in the block above or below it (the comments would be misleading), and it needs to go in between those two blocks. Lemme know if you have other suggestions though. -- To view, visit http://gerrit.cloudera.org:8080/2113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I616f46a861b4909d7a6e66dcf947b3518556768e Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
