Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10550 )
Change subject: IMPALA-7078: Part 1: improve memory consumption of wide Avro scans ...................................................................... Patch Set 7: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/exec/hdfs-avro-scanner.cc File be/src/exec/hdfs-avro-scanner.cc: http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/exec/hdfs-avro-scanner.cc@498 PS7, Line 498: starts off with AtCapacity() == true. under which case do we get here with row_batch already AtCapacity()? Not asking to restructure now, but why is it structured that way? http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/exec/hdfs-avro-scanner.cc@498 PS7, Line 498: mke make http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/runtime/mem-tracker.h@238 PS7, Line 238: Must not be used if do we have cases that transfer in this case? if so, what's the recommended pattern? (Release() and Consume()?) http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/10550/7/be/src/runtime/mem-tracker.cc@236 PS7, Line 236: && all_trackers_[ancestor_idx - 1] == dst->all_trackers_[dst_ancestor_idx - 1]) { should we dcheck that the limits are not set? -- To view, visit http://gerrit.cloudera.org:8080/10550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iebd2600b4784fd19696c9b92eefb7d7e9db0c80b Gerrit-Change-Number: 10550 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Jun 2018 23:17:08 +0000 Gerrit-HasComments: Yes
