impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions
Date Mon, 24 Oct 2016 18:04:33 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions

Patch Set 6:


Looking good. Some more comments for now. Still doing another pass.
File be/src/exec/

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,
              :   Value* dst_unlowered_ptr = builder->CreateBitCast(
              :       dst_lowered_ptr, dst_unlowered_ptr_type, "dst_unlowered_ptr");
Why not dst.GetUnloweredPtr() ?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id9dc21d1d676505d3617e1e4f37557397c4fb260
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message