Tim Armstrong has posted comments on this change. Change subject: IMPALA-2581: LIMIT can be propagated down into some aggregations ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/3822/3/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: PS3, Line 294: // /// PS3, Line 298: max_groups_ is checked via ReachedAggregationLimit() below. Maybe a little misleading. I think the important check is in the -ir.cc file before adding a new row to the hash table. ReachedAggregationLimit() is just an optimisation to return early. Line 299: // needed for limiting the number of rows returned, since the limit is applied in the could be a little clearer: something like "limiting the number of groups in a grouping aggregation is sufficient to limit the number of output rows". PS3, Line 671: /// Line 675: bool ReachedAggregationLimit() const { I think the name is misleading because it's not really enforcing the same thing as ReachedLimit() (your comment does mention this). ReachedLimit() which checks if the node has output enough rows. Whereas this is checking if it's consumed enough input rows. Maybe ConsumedEnoughInput()? http://gerrit.cloudera.org:8080/#/c/3822/3/fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java File fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java: PS3, Line 910: isLimitable isPreaggLimitable()? http://gerrit.cloudera.org:8080/#/c/3822/3/testdata/workloads/functional-query/queries/QueryTest/spilling.test File testdata/workloads/functional-query/queries/QueryTest/spilling.test: Line 632: # Test spilling an agg with a LIMIT; see IMPALA-2581 Were you able to reproduce the bug? I'd feel a lot more comfortable if we had test coverage for that case since it's easy to get wrong. -- To view, visit http://gerrit.cloudera.org:8080/3822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59c5b7af7a73ccdbc5496b28eacb9b6859d202bc Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
