Quanlong Huang 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) > 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. Thanks for checking other operators! I think if unfortunately there are similar bugs in other operators, we might need to fix them in the same way. So this is a suitable fix for the current bugs we found. Refactored the patch to not introduce the num_pinned_hash_partitions_ field. Added a test with random data for more coverage. 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 Removed this to reduce complexity. 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 coun Removed num_pinned_hash_partitions_ to reduce complexity. 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 f Sure. Don't want to complicate this patch. Added a method to calculate it when needed. -- 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: Mon, 24 Aug 2020 12:59:37 +0000 Gerrit-HasComments: Yes
