Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10421 )

Change subject: IMPALA-5168: Codegen HASH_PARTITIONED 
KrpcDataStreamSender::Send()
......................................................................


Patch Set 2:

(4 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:  
RETURN_IF_ERROR(partition_exprs_[i]->GetCodegendComputeFn(codegen, 
&compute_fn));
             :
             :     // 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);
> can we use Builder.CreateExtractValue() and get rid of this function call.
The above is actually the unoptimized version of the code. The reason I avoided 
using CreateExtractValue() or GetElementPtr() etc is to avoid the need to make 
assumption about the order of the fields inside the struct plus it's easier to 
review in general.

I confirmed in the optimized version of the code that this function call is 
actually inlined and optimized away as dead code.


http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@803
PS1, Line 803: t string sender_name = PartitionTypeName() + " Send
> nit: Invert condition to avoid indentation.
Done


http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@818
PS1, Line 818: ced;
> nit: HashRow()
Done


http://gerrit.cloudera.org:8080/#/c/10421/1/be/src/runtime/krpc-data-stream-sender.cc@820
PS1, Line 820: en->ReplaceCallSitesWithValue(hash_and_add_
> nit: maybe use a named const like
Done



--
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: 2
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 May 2018 22:11:49 +0000
Gerrit-HasComments: Yes

Reply via email to