Michael Ho has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 9: (4 comments) http://gerrit.cloudera.org:8080/#/c/1901/9/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 138: void MaterializeExprs This can be private. 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); > Just realized with the templates you would reintroduce the problem where bo I can imagine having an extra template parameter to indicate whether there is a pool or not like the following but not sure if it's any better: template <bool collect_string_val, bool use_pool> inline void IR_ALWAYS_INLINE MaterializeExprWork(...) { if (collect_string_vals) { } if (use_pool) { MaterializeExprs(...); } else { MaterializeExprsNoPool(...); } } With this many levels of templates, the code may be a bit confusing so I agree that we may not strictly need to have two different signatures if the functions' names are sufficient for differentiation. That may make code sharing easier. Your call. Line 162: : void IR_NO_INLINE MaterializeExprsNoPool This can be private. Line 166: MaterializeExprs<collect_string_vals>(row, desc, materialize_expr_ctxs, NULL, : non_null_string_values, total_string); The body of the function may not need to be in the header file. -- 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
