Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/1901/9/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 202: if (collect_string_vals) *total_string = 0; > Can this be moved to the body if (collect_string_vals) stmt of the caller i Done. I also made this and MaterializeExprsNoPool() below private, since they are only used for codegen. Line 326: // StringValue** non_null_string_values, int* total_string) > Please consider also adding the signature of the NoPool version. Done 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); > Why cannot this simply be MaterializeExprs<collect_string_vals>(row, desc, This function is inlined in the IR, and we look for this MaterializeExprsNoPool() call for replacement. It would be nice if we didn't have to duplicate all the above code. Maybe instead of having two different functions (MaterializeExprs and MaterializeExprsNoPool), we could add a template parameter specifying whether 'pool' is NULL, and then use the templatized symbol for replacement. What do you think? The template is kind of ugly/redundant, but we avoid this copy/paste. -- 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
