impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1430: enable codegen for native UDAs
Date Mon, 12 Dec 2016 23:03:28 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-1430: enable codegen for native UDAs
......................................................................


Patch Set 6:

(7 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);
I am not sure I understand the intention of this.

The Update() / Merge() functions of UDA are assumed to have the signature void Update(FunctionContext*,
ArgType1* arg1, ArgType2* arg2, ...., ResultType* result); If so, shouldn't the code which
calls Expr::GetConstantInt(Expr::RETURN_TYPE_SIZE); expect to get the size of void (if it's
defined at all) back ?

I can see replacement of Expr::RETURN_TYPE_SIZE happening due to inlining of other functions
in the Update()/Merge() functins but that doesn't seem to be correct all the time to replace
the return type with intermediate type. For example, one can call DecimalOperators::CastToDecimalVal()
on one of the input argument to the UDA's Update() function. Replacing the return type of
CastToDecimalVal() with the intermediate type of the AggFnEvaluator may not be correct.


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 327: !udf->isDeclaration()
May help to add a comment on why this can be a declaration only (i.e. no function body).


PS6, Line 328:     InlineConstants(AnyValUtil::ColumnTypeToTypeDesc(type_),
             :         AnyValUtil::ColumnTypesToTypeDescs(arg_types), codegen, udf);
Please see comments in AggFnEvaluator. It seems that we should be able to keep InlineConstants()
in GetUdf() like before. Of course, I could have misunderstood the purpose of the change in
AggFnEvaluator().


PS6, Line 449:   DCHECK(has_varargs || arg_types.size() == num_fixed_args);
             :   DCHECK(!has_varargs || arg_types.size() > num_fixed_args);
DCHECK(arg_types.size() == num_fixed_args ||  (has_var_args && arg_types.size() >
num_fixed_args)); ?


PS6, Line 478:         codegen->void_type() :
             :         CodegenAnyVal::GetLoweredType(codegen, *return_type);
nit: one line ?


http://gerrit.cloudera.org:8080/#/c/5161/6/be/src/exprs/scalar-fn-call.h
File be/src/exprs/scalar-fn-call.h:

PS6, Line 59: 'cache_entry'
May help to have a comment about what 'cache_entry' is. Something like the following, preferably
shorter.

'cache_entry' is the entry of the shared library cache owned by the caller. Loading the UDF
will increment the use count. Caller is expected to decrement the use count when they are
done with the UDF (e.g. in Expr::Close()).


Line 60:   /// updated to point to the library (or its use count is incremented if already
loaded).
Please add a comment that the caller is expected to call FinalizeFunction() on 'udf' to verify
it;


-- 
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message