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 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@348 PS8, Line 348: f the column. > We should be using Close() here instead of unregistering every tracker (thi Removed the Close() since I'm using the MemTracker of scan_node_->mem_tracker so won't be using 1000's of memtrackers now http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@133 PS8, Line 133: di > nit: inline here and nearby isn't necessary for a couple of reason. First, Removed inline http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@184 PS8, Line 184: Node(const T& v, const NodeIndex& n) : value(v), next(n) { } > Nit: check isn't necessary - Release() does this for you. Removed the check http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@194 PS8, Line 194: enum { INVALID_INDEX = 40000 }; > The TODO should be to use TryConsume(). The asynchronous checks are not the Changed the TODO to use TryConsume() http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@251 PS8, Line 251: / This function > I agree with Joe's comment. All codepaths should call Close(), so we can ju Added a DCHECK found during the testing that some of the column readers do not call Close, so I've added ReleaseBytes() so that accounting of memory used for dictionary is release when DictDecoder is destroyed. -- 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: 9 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 18 Oct 2017 05:28:47 +0000 Gerrit-HasComments: Yes
