Tim Armstrong has posted comments on this change. Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/6263/1//COMMIT_MSG Commit Message: Line 16: I did a grep for CastPtrToLlvmPtr() to make sure there were no other instances of this pattern. I noticed in scalar-fn-call.cc there's some dead code that uses this pattern - maybe we should just remove it while we're working on a related patch. http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table-ir.cc File be/src/exec/hash-table-ir.cc: Line 26: ExprContext* const* HashTableCtx::GetBuildExprCtxs() const { return build_expr_ctxs_.data(); } Long lines http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 736: Value* ctx_vector = builder.CreateCall(get_expr_ctx_fn, this_ptr, "ctx_vector"); Comment that this is pointer to the first ExprContext* pointer? Line 1099: Value* ctx_vector = builder.CreateCall(get_expr_ctx_fn, this_ptr, "ctx_vector"); Maybe comment that this is a pointer to the first ExprContext*? Line 1121: // Load ExprContext* from 'build_expr_ctxs_'. No longer accurate http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/old-hash-table.cc File be/src/exec/old-hash-table.cc: Line 651: Function* get_build_expr_ctx_fn = What do you think about adding a CallFunction() helper to LlvmCodegen to make the GetFunction()/CreateCall() pattern more concise. -- To view, visit http://gerrit.cloudera.org:8080/6263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
