impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message