Tim Armstrong has posted comments on this change.

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


Patch Set 8:

(4 comments)

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

Line 54: /// TYPE_DECIMAL/DecimalVal (isn't lowered):
> Please update this comment to include TYPE_FIXED_UDA_INTERMEDIATE
Done


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);
> It's a bit unfortunate that this is inconsistent with CodegenAnyVal::GetUnl
I think this makes sense so long as we make it clear that the function returns 
the internal representation of the type. It's only called with an arbitrary 
ColumnType from GetLlvmStruct() and from CodegenAnyVal, where it's expecting 
the get the native type as it's represented in the tuple.

I updated the comment.


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

PS8, Line 1541: //   call void @"void 
impala::AggregateFunctions::HllUpdate<impala_udf::TimestampVal>"(
              : //       %"class.impala_udf::FunctionContext"* %agg_fn_ctx,
              : //       %"struct.impala_udf::TimestampVal"* 
%input_unlowered_ptr,
              : //       %"struct.impala_udf::StringVal"* %dst_unlowered_ptr)
              : //   %anyval_result = load { i64, i8* }, { i64, i8* }* 
%dst_lowered_ptr
              : //   %7 = extractvalue { i64, i8* } %anyval_result, 0
              : //   %8 = ashr i64 %7, 32
              : //   %9 = trunc i64 %8 to i32
              : //   %10 = insertvalue %"struct.impala::StringValue" 
zeroinitializer, i32 %9, 1
              : //   %11 = extractvalue { i64, i8* } %anyval_result, 1
              : //   %12 = insertvalue %"struct.impala::StringValue" %10, i8* 
%11, 0
              : //   store %"struct.impala::StringValue" %12, 
%"struct.impala::StringValue"* %dst_slot_ptr
              : //   br label %ret
> Does this need some updating ?
Done


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:
> This seems a bit tricky as the Serialize/Finalize functions need to be awar
A DCHECK doesn't seem appropriate since a UDA could set the incorrect thing. 
Turns out that this code is reachable by declaring a UDA with char intermediate 
and a serialize function. I added a test with a CHAR intermediate that DCHECKs 
on master.

The other option that makes sense to me is to simply ignore the return value 
and not warn, which is what the codegen'd path does anyway.


-- 
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

Reply via email to