Dan Hecht has posted comments on this change. Change subject: IMPALA-1430: enable codegen for native UDAs ......................................................................
Patch Set 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/5161/13/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: Line 215: AnyValUtil::ColumnTypeToTypeDesc(input_expr_ctxs_[i]->root()->type())); this seems wrong for merge/finalize. in those cases, this will be the intermediate type, I believe, but we already have that available from GetIntermediateType(). It seems to me GetArgTypes() should always be the aggregate function's logical inputs. PS13, Line 535: ScalarFnCall seems like a strange place given this isn't a scalar function being loaded... PS13, Line 538: !(*uda_fn)->isDeclaration() what does this condition mean? that it's defined? if it's not defined, what does that mean -- that it's defined only in native code? PS13, Line 541: Constant inlining must therefore expose it in the same way. what is this saying? i can see why arg_types for InlineConstants() should only have the true arguments, but not sure what we're saying here. PS13, Line 547: arg_types as we discussed in person, it doesn't seem right to use these types for FunctionContext::GetArgType(), because this will be the intermediate type not the input types. i.e. GetArgTypes() currently returns different types depending on if executing in the update or the merge/finalize functions. This does match what we do when constructing the FunctionContext (but I'm proposing we change the Prepare() code). -- 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: 13 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@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
