Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10421 )
Change subject: IMPALA-5168: Codegen HASH_PARTITIONED KrpcDataStreamSender::Send() ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@718 PS1, Line 718: // Load the expression evaluator for the i-th partition expression : llvm::Function* get_expr_eval_fn = : codegen->GetFunction(IRFunction::KRPC_DSS_GET_PART_EXPR_EVAL, false); : DCHECK(get_expr_eval_fn != nullptr); : llvm::Value* expr_eval_arg = : builder.CreateCall(get_expr_eval_fn, {this_arg, codegen->GetI32Constant(i)}); can we use Builder.CreateExtractValue() and get rid of this function call. (Looks like something that the llvm optimizer could have inlined but didn't according to the sample generated code above) http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@803 PS1, Line 803: partition_type_ == TPartitionType::HASH_PARTITIONED nit: Invert condition to avoid indentation. if (partition_type_ != TPartitionType::HASH_PARTITIONED) { const string& msg = Substitute("not $0", partition_type_ == TPartitionType::KUDU ? "supported" : "needed"); profile()->AddCodegenMsg(false, msg, sender_name); return; } llvm::Function* hash_row_fn; codegen_status = CodegenHashRow(codegen, &hash_row_fn); . . . http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@818 PS1, Line 818: HashRows nit: HashRow() http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@820 PS1, Line 820: KrpcDataStreamSender7HashRowEPNS_8TupleRowE nit: maybe use a named const like const char* HASH_ROW_SYMBOL http://gerrit.cloudera.org:8080/#/c/10421/1/testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test File testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test: http://gerrit.cloudera.org:8080/#/c/10421/1/testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test@61 PS1, Line 61: ---- QUERY add cases for "codegen not supported/needed" -- To view, visit http://gerrit.cloudera.org:8080/10421 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c44cc9312c062cc7a5a4ac9156ceaa31fb887ff Gerrit-Change-Number: 10421 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Comment-Date: Wed, 16 May 2018 23:42:33 +0000 Gerrit-HasComments: Yes
