Alex Behm has posted comments on this change. Change subject: IMPALA-5713: always reserve memory for preaggs ......................................................................
Patch Set 4: Code-Review+1 (2 comments) FE changes lgtm. I only skimmed the BE code which also looked ok. Let's wait for Mostafa's input. http://gerrit.cloudera.org:8080/#/c/7739/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: PS3, Line 327: 16b > I guess the number of buckets doesn't matter too much from the frontend's p I vote for omitting that last calculation. Line 330: perInstanceMinReservation = (bufferSize + 64 * 1024) * PARTITION_FANOUT; > Yeah it was. That's one of the reasons I don't think this change is a win i Tim once suggested that the pre-agg should really be a separate exec node. I like that idea a lot since the pre-agg is such a critical step with special characteristics. Why even have 16 partitions etc. I prefer this patch over the old behavior because in most circumstances I believe users would be willing to give up a little memory to have their query performance be more robust. The really bad scenario where all rows are passed through could happen "sometimes" even for the same query depending on the state of the system, so I think getting a reservation is the right way to go. -- To view, visit http://gerrit.cloudera.org:8080/7739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b544f9ec1a906719e2bbb074743926b95a3a573 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
