Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 10: (7 comments) 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_, > why do you need to call two versions of this? the code behind it seems iden Not sure what you mean by two versions, are you referring to the second template parameter? That's to create different symbols for replacement during codegen, one for the pool=NULL call and one for the pool=tuple_pool call. http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 201: template <bool collect_string_vals, bool no_pool> > format as constants Done Line 352: // Value* desc_arg = args[2]; // unused > remove Done Line 354: // 'pool', 'non_null_string_values', and 'total_string' are unused > also mention desc Done Line 362: codegen->CodegenMemset(&builder, tuple, 0, desc.num_null_bytes()); > this seems unnecessary (and potentially dangerous, if we materialize a whol You can't materialize a tuple in increments using this function since it iterates through all the slots, so I think this is safe here (and puts fewer prerequisites on the caller). http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 123: template <bool collect_string_vals, bool no_pool> > these are effectively constants in the context of the function, so capitali I made it NULL_POOL so it's less negative. I think NULL_POOL's more explicit than NO_POOL or WITH_POOL, but lemme know if you prefer WITH_POOL. Line 148: /// generates an IR versin of MaterializeExprsNoPool(). > version Done. Also removed stale reference to MaterializeExprsNoPool(). -- 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 <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
