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: (5 comments) 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: IKELY(!new > nit: row_is_added Done http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator-partition.cc@144 PS8, Line 144: ervation(); > nit: probably more readable to make this a local variable instead of duplic Done 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: BufferPool::SubReservation large_write_page_reservation_; > Do we need the unique_ptr indirection? Seems like we could just have the Su Yeah, this saves some nullptr checks. Done. http://gerrit.cloudera.org:8080/#/c/16240/8/be/src/exec/grouping-aggregator.h@546 PS8, Line 546: /// the 'large_write_page_reservation_' when adding the last row. > Can you add comments to explain this argument. Done 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: DCHECK(!is_streaming_preagg_); : DCHECK(partition->is_spilled()); : BufferedTupleStream* stream = AGGREGATED_ROWS ? : partition->aggregated_row_stream.get() : : partition->unaggregated_row_stream.get(); : DCHECK(!stream->is_pinned()); : Status status; : if (LIKELY(AddRowToSpilledStream(stream, row, &status))) return Status::OK(); : RETURN_IF_ERROR(status); : : // Keep trying to free memory by spilling and retry AddRow() until we run out of : / > Can we factor out this code (i.e. AddRow(), then fall back to AddRowWithExt Yes! And after adding a field to track the number of pinned partitions, I can simplify these codes more. -- 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 02:22:44 +0000 Gerrit-HasComments: Yes
