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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
