Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr ...................................................................... Patch Set 2: (15 comments) Looks good to me! 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); Wonder if we need this. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@35 PS2, Line 35: Invalid TimestampValue: " + tv->ToString() Can we also include the function name in the message? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@45 PS2, Line 45: Invalid DateValue Same as above. Also, is it possible to include the invalid value itself? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h@88 PS2, Line 88: ConvertTimestampValue( Maybe reworded to 'ConvertTimestampValueToKudu()' to make it clear that the result is in Kudu format. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h@91 PS2, Line 91: ConvertDateValue Same as above. 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; Should we DCHEK(row && partitioner) here? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@44 PS2, Line 44: KuduPartitionRow nit: reworded as GetKuduPartitionRow(). http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.h File be/src/exprs/kudu-partition-expr.h: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.h@77 PS2, Line 77: static void SetKuduPartialRowAndPartitioner(ScalarExprEvaluator* eval, int fn_ctx_idx, A comment here will be nice. 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@122 PS2, Line 122: A comment here will help. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@152 PS2, Line 152: / This can only fail if we set a col to an incorrect type, which would be a bug in : // planning, so we could DCHECK but in codegen code we can't so we do not check it. This could happen when the impala::Status is not a class anymore. Highly unlikely. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@165 PS2, Line 165: A comment here will help. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@175 PS2, Line 175: llvm::Value* args[2]; Maybe add a comment on what these two arguments are for. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@206 PS2, Line 206: non_null_block maybe renamed as not_null_block http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@222 PS2, Line 222: (GetChild(i) Looks like we can directly use child_expr here. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@229 PS2, Line 229: llvm::BasicBlock* next_eval_child_block = : llvm::BasicBlock::Create(context, "eval_child", function); : builder.CreateBr(next_eval_child_block); : current_eval_child_block = next_eval_child_block; I wonder if this block is redundant when i == num_children - 1. -- 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: 2 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Thu, 10 Sep 2020 19:40:47 +0000 Gerrit-HasComments: Yes
