Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201603182201.u2IM1c1K026635@ip-10-146-233-104.ec2.internal>
Date: Fri, 18 Mar 2016 22:01:38 +0000
From: "Skye Wanderman-Milne (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Reply-To: skye@cloudera.com
Subject: 
 =?UTF-8?Q?[Impala-CR](cdh5-trunk)_IMPALA-2548:_Codegen_Tuple::MaterializeExprs()_and_use_in_TopN_node=0A?=
In-Reply-To: 
 <gerrit.1453770925000.Ib422a8d50303c21c6a228675157bf867e8619444@gerrit.cloudera.org>
References: 
 <gerrit.1453770925000.Ib422a8d50303c21c6a228675157bf867e8619444@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0

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