Michael Ho has posted comments on this change. Change subject: IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions ......................................................................
Patch Set 6: (7 comments) Looking good. Some more comments for now. Still doing another pass. 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() of various aggregate operators (e.g. initNull() and initZero()) so this is even true for the interpreted path, right ? Not sure why we cannot do that even for the interpreted path too ? PS6, Line 1617: Value* dst_ptr May help to explain what dst_ptr actually is ? It seems to be pointer to the slot for this agg_fn_evaluator in the intermediate tuple PS6, Line 1631: (agg_op == AggFnEvaluator::MIN || agg_op == AggFnEvaluator::MAX)) nit: I find it easier to read to check the agg_op first: if ((agg_op == ..) && dst_is_numeric_or_bool)) { ... } PS6, Line 1639: slot_desc->CodegenSetNullIndicator Can this be skipped if (!slot_desc->is_nullable()) ? Same for below. PS6, Line 1663: agg_op == AggFnEvaluator::MIN agg_op != OTHER ? or you wanna keep this list for clarity ? PS6, Line 1718: false); Why not just pass true and skip calling 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() ? -- 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
