Michael Ho 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 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::GetUnloweredType() but I can see you need to do so for TupleDescriptor::GetLlvmStruct(). Do you think it makes sense to special case in TupleDescriptor::GetLlvmStruct() instead and return a string_val_type_ here ? 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 ? 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 aware of this intermediate type and not copy the data. May be a DCHECK would help so we crash right away during development ? -- 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 <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
