Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16314 )
Change subject: IMPALA-7658: Proper codegen for HiveUdfCall ...................................................................... Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/16314/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16314/8//COMMIT_MSG@11 PS8, Line 11: TODO: Testing > I believe we should be covering all the data types and codegen enabled/disa The java tests pass locally, I'll run an exhaustive Jenkins test if this passes the initial tests. http://gerrit.cloudera.org:8080/#/c/16314/10/be/src/exprs/hive-udf-call-ir.cc File be/src/exprs/hive-udf-call-ir.cc: http://gerrit.cloudera.org:8080/#/c/16314/10/be/src/exprs/hive-udf-call-ir.cc@84 PS10, Line 84: if (type->type == TYPE_STRING) { I implemented string copying here instead of a separate cross-compiled 'CopyStringValToResultAlloc' function. This way the code is simpler and we have one less IR function. On the other hand, the type check becomes a runtime check if LLVM doesn't optimise it out. Do you think it might have a performance impact? I think even if it is not optimised out it is a predictable branch and the cost should not be significant compared to the call to Java. http://gerrit.cloudera.org:8080/#/c/16314/8/be/src/exprs/hive-udf-call.cc File be/src/exprs/hive-udf-call.cc: http://gerrit.cloudera.org:8080/#/c/16314/8/be/src/exprs/hive-udf-call.cc@179 PS8, Line 179: > Cache Done http://gerrit.cloudera.org:8080/#/c/16314/8/be/src/exprs/hive-udf-call.cc@278 PS8, Line 278: > I think this should return int64_t, since that's what we generally use for I moved it to codegen-util.h, but I'm not sure it is very codegen-related.. -- 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: 10 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 25 Aug 2020 07:42:49 +0000 Gerrit-HasComments: Yes
