Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8966 )
Change subject: IMPALA-4835: Part 3: switch I/O buffers to buffer pool ...................................................................... Patch Set 20: (10 comments) Rebased onto master to pick up Bikram's runtime filter change. Fixed tests to reflect min buffer size and the runtime filters. Also found some bugs in the nested types reservation calcs around non-materialized commons that were triggered by the lower minimum buffer size. Fixed those and added planner tests to verify the calculation in even more edge cases. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@222 PS19, Line 222: is stream for allocating I/O buffers. The > what about the reservation for reading the scan_range_? why does this "rea I tried to clarify the relationship between this and 'scan_range_' reservation a bit. This is a consequence of 'scan_range_' being created externally and passed in. I thought about trying to refactor this so that ScannerContext::Stream is responsible for allocating buffers for the scan range, i.e. so reservation is managed internally rather than having the main scan range treated differently, but it seemed to be biting off a bit too much. I think the old comment was misleading because it can be used for other scan ranges too - e.g. in Parquet the reservation from the initial (footer) stream is taken and divided between the column streams. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@223 PS19, Line 223: e this until : /// all > is that talking about the "read size" or "BP min buffer size"? Clarified. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@284 PS19, Line 284: > is this the "read past" reservation or something else? It's the same as 'reservation_'. Added a brief comment for the method. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@372 PS19, Line 372: /// end of 'range'. 'reservation' is shared with 'range's reservation: all of 'range's > this answers my question about constructor. Maybe reference this function Done http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.cc@153 PS19, Line 153: : // Allocate fresh bu > if that's true, why can't we reuse their reservation? Or maybe we actually Yeah that's right. Will look at header comments to see if I can make it less confusing. http://gerrit.cloudera.org:8080/#/c/8966/18/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/8966/18/be/src/runtime/io/request-ranges.h@65 PS18, Line 65: r* io_mgr, RequestContext* reader, > update. Done http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@155 PS19, Line 155: mpling. Null if not sampli > can't we delete this now? Done http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@186 PS19, Line 186: > a time? Done http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1330 PS19, Line 1330: : private Pair<Long, Long> computeReservation(int numColumnsReadFr > isn't this more about the min buffer size? Done http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1345 PS19, Line 1345: BitUtil > iomgrScanRangesPerFile. Actually, the PerFile isn't right either since mul Done -- To view, visit http://gerrit.cloudera.org:8080/8966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic09c6196b31e55b301df45cc56d0b72cfece6786 Gerrit-Change-Number: 8966 Gerrit-PatchSet: 20 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Feb 2018 20:12:14 +0000 Gerrit-HasComments: Yes
