Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/1901/9/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 151: DCHECK_EQ(materialize_expr_ctxs.size(), desc.slots().size()); : StringValue** non_null_string_values_array = NULL; : if (collect_string_vals) { : non_null_string_values->clear(); : non_null_string_values->reserve(desc.string_slots().size()); : non_null_string_values_array = non_null_string_values->data(); : } : MaterializeExprsNoPool<collect_string_vals>(row, desc, materialize_expr_ctxs.data(), : non_null_string_values_array, total_string); > I can imagine having an extra template parameter to indicate whether there I meant to make the codegen'd MaterializeExprs() function have the extra template parameter too. This means we no longer need to have separate function names, since the template parameter causes two distinct symbols to be emitted. I implemented this, and I think this is the best compromise between usability, code repetition, and likelihood to replace the wrong functions. The downsides are that callers need to specify the template parameter, which is redundant with the pool argument, and there's still the possibility of screwing up the replacement (although I created const strings for the symbol names to make this less likely). As per usual, lemme know if you think we should do something else. -- 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: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
