Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15463 )
Change subject: IMPALA-9530: query option to limit preagg memory ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/15463/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/15463/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@581 PS1, Line 581: if (useStreamingPreagg_ && queryOptions.getPreagg_bytes_limit() > 0) { > I think it would be better to have all the parameters set and then create the > ResourceProfileBuilder (similar to the previous code). It seems more > intuitive control flow. For MaxMemReservation which was not set earlier, can > we pass in -1 or 0 for no reservation ? One of the things I like about about the builder interface is that there's no need to pass in special values to indicate unset. There's some logic in the builder that depends on this (the max reservation is adjusted to LONG_MAX automatically when it's unset). I can work around that, but I tried changing this around and the control flow doesn't seem any simpler, so I didn't make the change in this PS. long maxReservationBytes = 0; // This is a magic value meaning "unset". if (useStreamingPreagg_ && queryOptions.getPreagg_bytes_limit() > 0) { maxReservationBytes = Math.max(perInstanceMinMemReservation, queryOptions.getPreagg_bytes_limit()); // Aggregations should generally not use significantly more than the max reservation, // since the bulk of the memory is reserved. perInstanceMemEstimate = Math.min(perInstanceMemEstimate, maxReservationBytes); } ResourceProfileBuilder builder = new ResourceProfileBuilder() .setMemEstimateBytes(perInstanceMemEstimate) .setMinMemReservationBytes(perInstanceMinMemReservation) .setMaxMemReservationBytes(maxReservationBytes) .setSpillableBufferBytes(bufferSize) .setMaxRowBufferBytes(maxRowBufferSize); > Also, just to clarify .. should the spillable buffer size and max row buffer > size be upper bounded by pregg_bytes_limit ? Since the calculation for these > did not consider preagg_bytes_limit, could there be cases where it is > exceeded ? (although perhaps the backend makes some adjustments on its own). This is handled by taking the max of this and the min reservation - the min reservation is guaranteed to be enough to allocate all of the buffers required. I am missing a test for this though, so I'll add one. -- To view, visit http://gerrit.cloudera.org:8080/15463 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I87f7a5c68da93d068e304ef01afbcbb0d56807d9 Gerrit-Change-Number: 15463 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 18 Mar 2020 16:47:46 +0000 Gerrit-HasComments: Yes
