Tim Armstrong has posted comments on this change. Change subject: IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/4655/6/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS6, Line 1063: if we initialize the destination value to 0 (with the NULL bit set) > If I understand it correctly, this initialization is done at the initFn() o We could do that but it doesn't seem worth it (the interpreted path has enough additional overhead that I doubt it makes a measurable difference). PS6, Line 1617: Value* dst_ptr > May help to explain what dst_ptr actually is ? It seems to be pointer to th Done PS6, Line 1631: (agg_op == AggFnEvaluator::MIN || agg_op == AggFnEvaluator::MAX)) > nit: I find it easier to read to check the agg_op first: Done PS6, Line 1639: slot_desc->CodegenSetNullIndicator > Can this be skipped if (!slot_desc->is_nullable()) ? Same for below. Min/Max should always be nullable (if there are no input values, the output is NULL). Added a DCHECK to assert this. PS6, Line 1663: agg_op == AggFnEvaluator::MIN > agg_op != OTHER ? or you wanna keep this list for clarity ? Yeah I prefer this way, also since it won't break if someone adds a new aggregate op PS6, Line 1718: false); > Why not just pass true and skip calling CloneFunction() below ? I somehow missed that we called Clonefunction() below. PS6, Line 1742: Value* dst_lowered_ptr = dst.GetLoweredPtr("dst_lowered_ptr"); : Type* dst_unlowered_ptr_type = CodegenAnyVal::GetUnloweredPtrType(codegen, dst_type); : Value* dst_unlowered_ptr = builder->CreateBitCast( : dst_lowered_ptr, dst_unlowered_ptr_type, "dst_unlowered_ptr"); > Why not dst.GetUnloweredPtr() ? We use both dst_unlowered_ptr and dst_lowered_ptr so we need both variables. We can't just call both GetLoweredPtr() and GetUnloweredPtr() here because dst_lowered_ptr and dst_unlowered_ptr need to point to the same alloca(). -- To view, visit http://gerrit.cloudera.org:8080/4655 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9dc21d1d676505d3617e1e4f37557397c4fb260 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
