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 8: (5 comments) I think this makes sense. It does add complexity unfortunately but I didn't see a way to make it significantly simpler http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator-partition.cc File be/src/exec/grouping-aggregator-partition.cc: http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator-partition.cc@130 PS8, Line 130: rowIsAdded nit: row_is_added http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator-partition.cc@144 PS8, Line 144: reinterpret_cast<TupleRow*>(&tuple nit: probably more readable to make this a local variable instead of duplicating the cast http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.h File be/src/exec/grouping-aggregator.h: http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.h@286 PS8, Line 286: std::unique_ptr<BufferPool::SubReservation> large_write_page_reservation_; Do we need the unique_ptr indirection? Seems like we could just have the SubReservation and check is_closed() to see if it's initialised or not. http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.h@546 PS8, Line 546: HashTableCtx* ht_ctx, bool has_more_rows) WARN_UNUSED_RESULT; Can you add comments to explain this argument. http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.cc@639 PS8, Line 639: Status status; : if (LIKELY(stream->AddRow(row, &status))) return Status::OK(); : RETURN_IF_ERROR(status); : : // We fail to add a large row due to run out of unused reservation and fail to increase : // the reservation. If we don't have the serialize stream, spilling partitions don't : // need extra reservation so we can restore the large write page reservation (if we : // haven't done it) before spilling any partitions. : if (!needs_serialize_ && large_write_page_reservation_->GetReservation() > 0) { : if (LIKELY(AddRowWithExtraReservation(stream, row, &status))) return Status::OK(); : RETURN_IF_ERROR(status); : } Can we factor out this code (i.e. AddRow(), then fall back to AddRowWithExtraReservation) into a function. It is repeated 2x -- 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: 8 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: Thu, 13 Aug 2020 00:34:55 +0000 Gerrit-HasComments: Yes
