Pranay Singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/8034 )
Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder ...................................................................... Patch Set 16: (10 comments) http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@54 PS15, Line 54: DCHECK(buffered_indices_.empty()); > Ok to leave for now but we've generally been moving towards using the recen Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@59 PS15, Line 59: / This > DCHECK_EQ(dict_bytes_cnt_, 0); Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@77 PS15, Line 77: /// indices. Used to size the buffer passed to WriteData(). > To add to what Tim said, this code didn't actually change, so it is nice to Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@77 PS15, Line 77: /// indices. Used to size the buffer passed to WriteData(). > We generally use the more concise one-line formatting for very short single Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@123 PS15, Line 123: > formatting nit: * goes on the left with the type name. Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@152 PS15, Line 152: } : num_call_track_++; : } : } : }; > I think we can omit this destructor, since DictEncoderBase does the same th Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@168 PS15, Line 168: /// th > nit: inline isn't necessary. It isn't harmful but can be confusing to have Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@269 PS15, Line 269: > nit: * should be on left. Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@296 PS15, Line 296: : template<typename T> : class DictDecoder : public DictDecoderBase { : public: : // > I think we can omit this destructor, as DictDecoderBase does this. Done http://gerrit.cloudera.org:8080/#/c/8034/15/be/src/util/dict-encoding.h@330 PS15, Line 330: /// It > nit: unnecessary inline Done -- To view, visit http://gerrit.cloudera.org:8080/8034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299 Gerrit-Change-Number: 8034 Gerrit-PatchSet: 16 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 10 Nov 2017 06:22:29 +0000 Gerrit-HasComments: Yes
