You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/01/05 02:35:37 UTC
[Impala-ASF-CR] IMPALA-1430: enable codegen for native UDAs
Michael Ho has posted comments on this change.
Change subject: IMPALA-1430: enable codegen for native UDAs
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5161/6/be/src/exprs/agg-fn-evaluator.cc
File be/src/exprs/agg-fn-evaluator.cc:
PS6, Line 542: Expr::InlineConstants(AnyValUtil::ColumnTypeToTypeDesc(intermediate_type()),
: AnyValUtil::ColumnTypesToTypeDescs(arg_types), codegen, *uda_fn);
> This is preserving the pre-existing behaviour of the UDA interface, which t
My comment wasn't advocating for the change of UDA interface but instead questioning the validity of calling Expr::InlineConstants() here.
In particular, Expr::InlineConstants() was simply searching for call instructions in the UDA function and replacing those with callee with substring _ZN6impala4Expr14GetConstantInt with some constants. What if there is a new built-in UDA Update function likes the following:
void some_update(FunctionContext* ctx, IntVal* arg1, StringVal *dst) {
.....
DecimalVal d_val = DecimalOperator::CastToDecimalVal(ctx, arg1);
...
*dst = ....
}
In the code above, will Expr::InlineConstants() cause incorrect replacement for Expr::GetConstantInt() called in CastToDecimalVal() ?
http://gerrit.cloudera.org:8080/#/c/5161/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
PS6, Line 328: InlineConstants(AnyValUtil::ColumnTypeToTypeDesc(type_),
: AnyValUtil::ColumnTypesToTypeDescs(arg_types), codegen, udf);
> That was the initial path I took but it's not possible because UDAs treat t
I will hold off from commenting on this until we resolve the question in AggFnEvaluator.
PS6, Line 449: DCHECK(has_varargs || arg_types.size() == num_fixed_args);
: DCHECK(!has_varargs || arg_types.size() > num_fixed_args);
> We actually want to reject the case of passing 0 varargs into a varargs fun
Ah ! This is really a subtle corner case.
PS6, Line 478: codegen->void_type() :
: CodegenAnyVal::GetLoweredType(codegen, *return_type);
> clang-format seems to prefer it this way. I don't feel strongly
Can you please keep it on a single line ?
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes