Michael Ho has posted comments on this change. Change subject: IMPALA-1430: enable codegen for native UDAs ......................................................................
Patch Set 10: (7 comments) Looking good. Most comments about comments. http://gerrit.cloudera.org:8080/#/c/5161/10/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: PS10, Line 539: // The last (in/out) argument is exposed to UDAs via the FunctionContext as the : // return type, rather than as the last argument type. Constant inlining must : // therefore expose it in the same way. May help to add a remark in aggregate-functions-ir.cc about the assumption that the type of the last argument (the intermediate value) is treated as return type instead of argument type from the perspective of Expr::InlineConstants(). Otherwise, this can easily lead to confusion when writing built-in UDA. I think all use cases of Expr::InlineConstants() for built-in UDAs now are for substituting the arguments' types. On the other hand, if we don't consider the last argument as the return value but instead as a value which will be updated by the UDA, these logic can be hidden inside ScalarFnCall::GetUdf(). Not sure if we are getting much by using the current convention in the patch. http://gerrit.cloudera.org:8080/#/c/5161/10/be/src/exprs/scalar-fn-call.h File be/src/exprs/scalar-fn-call.h: PS10, Line 53: or HDFS or external module ? PS10, Line 54: UDF It could also be the update/merge function of a UDA, right ? Not sure if it makes sense to call it a UDF in that case. PS10, Line 61: nit: extra blank space. http://gerrit.cloudera.org:8080/#/c/5161/10/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: PS10, Line 51: UDAF May be less confusing to say "UDA functions" PS10, Line 52: an Expr node : /// generated by the Impala frontend May be it helps to differentiate the ExprNode in the FE and BE b stating: "TExprNode generated by the Impala frontend" PS10, Line 54: Backend code should not create FunctionContexts for other : /// purposes FunctionContexts should entirely be allocated and managed by ExprContext. Exprs shouldn't try to create FunctionContext themselves. -- To view, visit http://gerrit.cloudera.org:8080/5161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
