Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9002 )
Change subject: IMPALA-6383: free memory after skipping parquet row groups ...................................................................... Patch Set 1: (3 comments) Is there a way to test this? http://gerrit.cloudera.org:8080/#/c/9002/1/be/src/exec/hdfs-parquet-scanner.h File be/src/exec/hdfs-parquet-scanner.h: http://gerrit.cloudera.org:8080/#/c/9002/1/be/src/exec/hdfs-parquet-scanner.h@647 PS1, Line 647: /// were returned. and when returning the last batch. nit: remove "."? http://gerrit.cloudera.org:8080/#/c/9002/1/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/9002/1/be/src/exec/hdfs-parquet-scanner.cc@623 PS1, Line 623: // We skipped the previous row group. Release resources before starting another. This gets called for cases where no resources have been reserved (e.g. L655). Should we add a brief comment here or in the header explaining that it is safe and cheap to do so? Could we even call it every time? And should we have DCHECKs before the loop to enforce that no resources are already used? http://gerrit.cloudera.org:8080/#/c/9002/1/be/src/exec/hdfs-parquet-scanner.cc@738 PS1, Line 738: col_reader->Close(row_batch); nit: while you're here you could reduce this to a single line. -- To view, visit http://gerrit.cloudera.org:8080/9002 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc2f8f27c9b238be60261539f8d4be2facb57a2b Gerrit-Change-Number: 9002 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Thu, 11 Jan 2018 14:23:26 +0000 Gerrit-HasComments: Yes
