impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1430,IMPALA-4108: codegen all builtin aggregate functions
Date Mon, 24 Oct 2016 20:16:03 GMT
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 <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