Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201612122303.uBCN3S09016580@ip-10-146-233-104.ec2.internal>
Date: Mon, 12 Dec 2016 23:03:28 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Marcel Kornacker <marcel@cloudera.com>
Reply-To: kwho@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-1430=3A_enable_codegen_for_native_UDAs=0A?=
In-Reply-To: <gerrit.1479748324000.Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f@gerrit.cloudera.org>
References: <gerrit.1479748324000.Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 12 Dec 2016 23:04:07 -0000

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