Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711100622.vAA6MUUh019126@ip-10-146-233-104.ec2.internal>
Date: Fri, 10 Nov 2017 06:22:29 +0000
From: "Pranay Singh (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Joe McDonnell <joemcdonnell@cloudera.com>,
        Bikramjeet Vig <bikramjeet.vig@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5522=3AUse_tracked_memory_for_DictDecoder_and_DictEncoder=0A?=
In-Reply-To: <gerrit.1505172487000.I02a3b54f6c107d19b62ad9e1c49df94175964299@gerrit.cloudera.org>
References: <gerrit.1505172487000.I02a3b54f6c107d19b62ad9e1c49df94175964299@gerrit.cloudera.org>
Reply-To: impala-cr@cloudera.com, marcelk@gmail.com, tarmstrong@cloudera.com,
        joemcdonnell@cloudera.com, reviews@impala.incubator.apache.org,
        bikramjeet.vig@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="KS9FCRHkPds="; charset=UTF-8
archived-at: Fri, 10 Nov 2017 06:22:37 -0000

--KS9FCRHkPds=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Pranay Singh has posted comments on this change=2E ( http://gerrit=2Ecloude=
ra=2Eorg:8080/8034 )

Change subject: IMPALA-5522:Use tracked memory for Di=
ctDecoder and DictEncoder
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E


Patch Set 16:

(10 comments)

http://gerrit=2Ecloudera=2Eorg=
:8080/#/c/8034/15/be/src/util/dict-encoding=2Eh
File be/src/util/dict-encod=
ing=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/src/util/dict-=
encoding=2Eh@54
PS15, Line 54:     DCHECK(buffered_indices_=2Eempty());
> O=
k to leave for now but we've generally been moving towards using the recen
=
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/src/util/dict-enc=
oding=2Eh@59
PS15, Line 59: / This
> DCHECK_EQ(dict_bytes_cnt_, 0);
Done


=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/src/util/dict-encoding=
=2Eh@77
PS15, Line 77:   /// indices=2E Used to size the buffer passed to W=
riteData()=2E
> To add to what Tim said, this code didn't actually change, =
so it is nice to
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/=
src/util/dict-encoding=2Eh@77
PS15, Line 77:   /// indices=2E Used to size =
the buffer passed to WriteData()=2E
> We generally use the more concise one=
-line formatting for very short single
Done


http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8034/15/be/src/util/dict-encoding=2Eh@123
PS15, Line 123: 
> for=
matting nit: * goes on the left with the type name=2E
Done


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8034/15/be/src/util/dict-encoding=2Eh@152
PS15, =
Line 152:       }
              :       num_call_track_++;
              : =
    }
              :   }
              : };
> I think we can omit this des=
tructor, since DictEncoderBase does the same th
Done


http://gerrit=2Eclou=
dera=2Eorg:8080/#/c/8034/15/be/src/util/dict-encoding=2Eh@168
PS15, Line 16=
8: /// th
> nit: inline isn't necessary=2E It isn't harmful but can be conf=
using to have 
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/sr=
c/util/dict-encoding=2Eh@269
PS15, Line 269: 
> nit: * should be on left=2E=

Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/15/be/src/util/dict-en=
coding=2Eh@296
PS15, Line 296: 
              : template<typename T>
      =
        : class DictDecoder : public DictDecoderBase {
              :  pub=
lic:
              :   //
> I think we can omit this destructor, as DictDec=
oderBase does this=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8034/1=
5/be/src/util/dict-encoding=2Eh@330
PS15, Line 330: /// It
> nit: unnecessa=
ry inline
Done


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/80=
34
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerr=
it-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Ge=
rrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Num=
ber: 8034
Gerrit-PatchSet: 16
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: B=
ikramjeet Vig <bikramjeet=2Evig@cloudera=2Ecom>
Gerrit-Reviewer: Joe McDonn=
ell <joemcdonnell@cloudera=2Ecom>
Gerrit-Reviewer: Pranay Singh
Gerrit-Revi=
ewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Fri, 1=
0 Nov 2017 06:22:29 +0000
Gerrit-HasComments: Yes

--KS9FCRHkPds=--