impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Skye Wanderman-Milne (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node
Date Fri, 18 Mar 2016 22:01:38 GMT
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 <skye@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message