Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16314 )
Change subject: IMPALA-7658: Proper codegen for HiveUdfCall ...................................................................... Patch Set 12: Code-Review+1 (5 comments) Only nits and small improvement ideas, can go in from my side. http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.h File be/src/exprs/hive-udf-call.h: http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.h@109 PS12, Line 109: ans typo: and http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.cc File be/src/exprs/hive-udf-call.cc: http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.cc@295 PS12, Line 295: // Child is null. : builder->SetInsertPoint(child_null_block); : builder->CreateCall(set_input_null_buff_elem_fn, : {jni_ctx, codegen->GetI32Constant(i), codegen->GetI8Constant(1)}); : builder->CreateBr(next_eval_child_block); : : // Child is not null. : builder->SetInsertPoint(child_not_null_block); : builder->CreateCall(set_input_null_buff_elem_fn, : {jni_ctx, codegen->GetI32Constant(i), codegen->GetI8Constant(0)}); optional: couldn't it be faster if the value of child_wrapped.GetIsNull("child_is_null") was passed directly to set_input_null_buff_elem_fn instead of branching? http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.cc@311 PS12, Line 311: ; nit: extra ; http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.cc@490 PS12, Line 490: llvm::Value* const jni_env = builder.CreateCall(get_jni_env_fn, {jni_ctx}, "jni_env"); Do we need to do this here, can't CallJavaAndStoreResult handle it, as jni_ctx is already passed? http://gerrit.cloudera.org:8080/#/c/16314/12/be/src/exprs/hive-udf-call.cc@508 PS12, Line 508: llvm::Value* const hdfs_location = : codegen->GetStringConstant(&builder, fn_.hdfs_location); : llvm::Value* const symbol_name = : codegen->GetStringConstant(&builder, fn_.scalar_fn.symbol); Can't we move these as members to JniContext instead of passing them during function call? I think it is simpler to do this in "normal" code than here, and speed shouldn't matter as these are only used in the error branch. -- To view, visit http://gerrit.cloudera.org:8080/16314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f994dac550f297ed3c88491816403f237d4d747 Gerrit-Change-Number: 16314 Gerrit-PatchSet: 12 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 27 Aug 2020 15:30:29 +0000 Gerrit-HasComments: Yes
