impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Skye Wanderman-Milne (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node
Date Wed, 13 Apr 2016 18:01:08 GMT
Skye Wanderman-Milne has posted comments on this change.

Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/exec/topn-node-ir.cc
File be/src/exec/topn-node-ir.cc:

Line 32:     insert_tuple->MaterializeExprs<false, false>(input_row, *materialized_tuple_desc_,
> i get that, but why is that needed? why do you need two different bodies, i
We don't need two different bodies for the codegen'd MaterializeExprs() function, just two
different symbols. However, we do need two different bodies for the inlined MaterializeExprs()
wrapper functions that deal with the vector operations, since they need to call the correct
inner function.

To see what I mean, I originally had made two different functions, MaterializeExprs() and
MaterializeExprsNoPool(). However, this caused a lot of copy/paste code as it required making
two versions of the inlined wrapper methods, see http://gerrit.cloudera.org:8080/#/c/1901/9/be/src/runtime/tuple.h.
There's some discussion between me and Michael on this at L159.


-- 
To view, visit http://gerrit.cloudera.org:8080/1901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib422a8d50303c21c6a228675157bf867e8619444
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message