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-3931: arbitrary fixed-size uda intermediate types
Date Mon, 14 Aug 2017 17:48:34 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
......................................................................


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS8, Line 467: // Represent this as an array of bytes.
             :       return ArrayType::get(GetType(TYPE_TINYINT), type.len);
> I think this makes sense so long as we make it clear that the function retu
IMHO, this change in behavior makes GetType() an error prone interface. Given there is no
clear guideline on which one to use, it's likely that one may call this function when generating
a signature for a IR function or any call sites which use both GetType() and GetUnloweredType()
may result in surprises. It seems safer to have a separate function for the internal representation.

We should also document clearly how FIXED_UDA_INTERMEDIATE is represented in IR. For instance,
we are mostly exposing it as a StringVal but when is it appropriate to represent it as an
ArrayType.


http://gerrit.cloudera.org:8080/#/c/7526/11/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS11, Line 1540:     bitcast { i64, i8* }* %dst_lowered_ptr to %"struct.impala_udf:
This seems unnecessary now but I guess DCE will eliminate it.


http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/exprs/agg-fn-evaluator.cc
File be/src/exprs/agg-fn-evaluator.cc:

PS8, Line 478: TYPE_FIXED_UDA_INTERMEDIATE:
> A DCHECK doesn't seem appropriate since a UDA could set the incorrect thing
Is TYPE_FIXED_UDA_INTERMEIDATE limited to built-in UDA only ?

Btw, did we not support TYPE_CHAR as intermediate type before ? Is it a change in behavior
?


-- 
To view, visit http://gerrit.cloudera.org:8080/7526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@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