Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/15463 )
Change subject: IMPALA-9530: query option to limit preagg memory ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Changes LGTM. 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 >> 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. I see...yeah in that case keeping the code as-is is fine. Thanks for adding the test for the min reservation. -- 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: 2 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 17:12:18 +0000 Gerrit-HasComments: Yes
