Michael Ho has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 9: (3 comments) Some more comments. I think we are close. 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 in the header file so the relevant parameters are initialized together ? Line 326: // StringValue** non_null_string_values, int* total_string) Please consider also adding the signature of the NoPool version. 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, materialize_expr_ctxs, NULL, non_null_string_values, total_string); ? -- 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
