Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr ...................................................................... Patch Set 3: (5 comments) Thanks a lot for the explanation. Appreciate it. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@32 PS2, Line 32: DCHECK(success); > I copied these functions from kudu-util.cc into this new file, it already h My understanding is that DCHECK is applied even in release code. If success is false, we will stop here and bail out, right? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@35 PS2, Line 35: Invalid TimestampValue in function Convert > You mean the name of this conversion function? I added it to the message. Yes. The idea is that we can know for sure which function is involved in the message. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc File be/src/exprs/kudu-partition-expr-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@40 PS2, Line 40: *row = kudu_row; : *partitioner = kudu_partitioner; > Why do you think it's better? Just to make sure these two pointers are always valid to assign values to. Maybe they are every time? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc File be/src/exprs/kudu-partition-expr.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@152 PS2, Line 152: lvm::Function* const write_kudu_fn = : codegen->GetFunction(IRFunction::WRITE_KUDU_VALUE, false); > I don't quite understand it, could you help me? I mean people changes the declaration of Impala::Status. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@229 PS2, Line 229: tus KuduPartitionExpr::GetCodegendComputeFnImpl( : LlvmCodeGen* codegen, llvm::Function** fn) { : llvm::Function* const kudu_partition_row_fn = : codegen->GetFunction(IRFunction::GET_KUDU_PARTI > It's not redundant, 'next_eval_child_block' is assigned to 'current_eval_ch Okay. Thanks for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 14 Sep 2020 14:07:15 +0000 Gerrit-HasComments: Yes
