Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16240 )
Change subject: IMPALA-9955,IMPALA-9957: Fix not enough reservation for large pages in GroupingAggregator ...................................................................... Patch Set 9: (3 comments) Ok, I think I'm nearly happy with this. I thought a bit more about the other operators and I don't think there are similar bugs. AnalyticEvalNode and the plan root sink only have single streams, so there's no complicated logic moving reservations between streams. PartitionedHashJoinNode doesn't have the same problem here - it can always immediately unpin a stream to spill each partition. I.e. it doesn't have the problem where it needs to allocate additional memory to spill a partition. http://gerrit.cloudera.org:8080/#/c/16240/9/be/src/exec/grouping-aggregator-partition.cc File be/src/exec/grouping-aggregator-partition.cc: http://gerrit.cloudera.org:8080/#/c/16240/9/be/src/exec/grouping-aggregator-partition.cc@234 PS9, Line 234: --parent->num_pinned_hash_partitions_; If we were going to keep maintaining this, I think we'd want a wrapper that decremented this along with setting hash_tbl to null, to maintain the invariant that it's equal to the number of partitions where is_spilled() is true. http://gerrit.cloudera.org:8080/#/c/16240/9/be/src/exec/grouping-aggregator-partition.cc@243 PS9, Line 243: void GroupingAggregator::Partition::Close(bool finalize_rows) { Do we need to decrement num_pinned_hash_partitions here too, if it was counted as pinned before? I looked and I guess by the time we're closing partitions we won't spill any more, so it might not matter as far as the code working, but it's confusing if the count can get out of sync. http://gerrit.cloudera.org:8080/#/c/16240/9/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/16240/9/be/src/exec/grouping-aggregator.cc@651 PS9, Line 651: while (num_pinned_hash_partitions_ > 0) { I think I actually preferred computing num_pinned_hash_partitions in this function because there was less state to reason about - I don't have to check that the counter is updated whenever a partition is spilled.. I think num_pinned_hash_partitions_ gets inconsistent when you close the hash partitions so it's a little confusing. -- To view, visit http://gerrit.cloudera.org:8080/16240 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d9c3a2e7f0da60071b920dec979729e86459775 Gerrit-Change-Number: 16240 Gerrit-PatchSet: 9 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 14 Aug 2020 20:33:52 +0000 Gerrit-HasComments: Yes
