impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2581: LIMIT can be propagated down into some aggregations
Date Thu, 11 Aug 2016 00:00:30 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2581: LIMIT can be propagated down into some aggregations

Patch Set 3:

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 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()?
File fe/src/main/java/com/cloudera/impala/planner/

PS3, Line 910: isLimitable
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I59c5b7af7a73ccdbc5496b28eacb9b6859d202bc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message