Skye Wanderman-Milne has posted comments on this change.

Change subject: IMPALA-2784: codegen 
PartitionedHashJoinNode::Partition::BuildHashTable()
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/2113/6/be/src/exec/partitioned-hash-join-node-ir.cc
File be/src/exec/partitioned-hash-join-node-ir.cc:

Line 291:         // TODO: codegen expr evaluation and hashing
> how hard is this? (can we attempt that for 2.6?)
Should be quite easy with the IRBuilder. I filed 
https://issues.cloudera.org/browse/IMPALA-3423


http://gerrit.cloudera.org:8080/#/c/2113/6/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 196:   bool insert_codegen_enabled = false;
> adapt name to what you call it below (something with ht construction?)
Done


http://gerrit.cloudera.org:8080/#/c/2113/6/be/src/exec/partitioned-hash-join-node.h
File be/src/exec/partitioned-hash-join-node.h:

Line 280:   Status CodegenInsertBatch(RuntimeState* state, llvm::Function* 
hash_fn,
> move this into Partition?
Personally I think this is simpler, because the codegen'd function pointers 
still need to live in PartitionedHashJoinNode (since there are multiple 
Partitions per PHJ node) and the PHJ node drives all the codegen. I could make 
just this function a static Partiton function though and pass in/out the 
appropriate PHJ node member variables.


-- 
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: 6
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

Reply via email to