Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609132004.u8DK4kKZ022095@ip-10-146-233-104.ec2.internal>
Date: Tue, 13 Sep 2016 20:04:46 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4008=3A_don=27t_bake_in_hash_table_and_hash_join_pointers=0A?=
In-Reply-To: <gerrit.1473266173000.Ie353666dbb5c958f0094d169306fe930ec3014c5@gerrit.cloudera.org>
References: <gerrit.1473266173000.Ie353666dbb5c958f0094d169306fe930ec3014c5@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.12.2
archived-at: Tue, 13 Sep 2016 20:04:51 -0000

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4326/4/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

Line 308:     uint8_t* ALWAYS_INLINE cur_expr_values() const { return cur_expr_values_; }
let's add a short comment here too:
/// Returns the current row's expression buffer. 
or something.


PS4, Line 310: .
for the current row.


PS4, Line 400: into
This one should have stayed "in" (or "of"), right? i.e. it doesn't write *expr_values.
(Could make them 'const uint8_t*' to clarify that too).


Line 427:   uint32_t HashVariableLenRow(uint8_t* expr_values, uint8_t* expr_values_null) const;
how about making these const as well.


Line 441:       TupleRow* build_row, uint8_t* expr_values, uint8_t* expr_values_null) const;
these too could be const to clarify they are "in" params.


-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes