Is your feature request related to a problem or challenge?
An AggregateExec can carry a limit hint (LimitOptions) that the aggregate cannot actually execute. Nothing checks this when the plan is built, so each of these plans builds fine and only misbehaves when it runs:
| plan |
what happens when it runs |
| limit, no MIN/MAX aggregate, no ordering direction |
internal_err!("Ordering direction required for DISTINCT with limit") |
| limit with more than one group by expression |
aggr.group_expr().expr()[0] panics |
limit of 0 on a top-k aggregate |
worst_val().expect("Missing root") panics: a queue of capacity 0 reports itself full with an empty root |
limit on an aggregate with a FILTER |
GroupedTopKAggregateStream never reads filter_expr, so the filter is silently dropped and the query returns wrong results |
limit on a COUNT/AVG/… aggregate |
the grouped streams stop reading input once they hold enough groups, so later batches falling into groups already accumulated are never read and the aggregate values come back short |
| limit whose ordering direction contradicts its MIN/MAX aggregate |
the top-k stream takes its direction from the aggregate and ignores the one on the limit, so it keeps the wrong K groups |
| limit with an unsupported key/value type |
debug_assert! in debug builds only |
None of these are reachable from SQL: the optimizer rules that push a limit into an aggregate check the conditions first. They are reachable by any other rule that copies a limit onto a different node, by an external consumer of the physical plan API, and by decoding a plan from protobuf.
Describe the solution you'd like
Check the limit against the shape of the aggregate in AggregateExecBuilder::build (added in #25376), which is the one place an AggregateExec is constructed. build already returns a Result and every caller already handles it: the two optimizer rules that push a limit down use build().ok()?, so a rejected limit means "skip this optimization" rather than a failed plan.
Three shapes accept a limit:
- an aggregate with no group by expressions: it produces a single row, so the limit is ignored;
- a
SELECT DISTINCT-style aggregate: a group by, no aggregate expressions, no FILTER, and no ordering direction on the limit;
- a top-k aggregate, which takes its ordering direction from a single
MIN/MAX expression, from the limit, or from both, and which also needs a limit above 0, exactly one group by expression and no grouping sets, no FILTER, and a group key and value type the top-k queue supports.
Describe alternatives you've considered
Writing the check as a list of the failure modes above is what #25376 first did, and it took three rounds of review to find the cases the list was missing in both directions — too loose on COUNT, too strict on a plan execute_typed would have run. The check should be derived from AggregateExec::execute_typed's own dispatch, ideally sharing code with it, so the two cannot drift: rejecting a limit the executor would have run turns a working query into a planning error.
It should also be tested by building each shape, executing it, and comparing against the same plan without the limit. The full sqllogictest suite passes with no check at all, because the optimizer never produces the plans the check exists to reject, so it carries no signal here.
A narrower alternative is to make two of these unrepresentable instead of merely rejected, by splitting LimitOptions into SoftLimit { limit } and TopK { limit, descending }. That is worth doing as well, and is a wider rename.
Additional context
Carved out of #25376, which is a pure refactor and does not change behaviour. Related: #25257.
Is your feature request related to a problem or challenge?
An
AggregateExeccan carry a limit hint (LimitOptions) that the aggregate cannot actually execute. Nothing checks this when the plan is built, so each of these plans builds fine and only misbehaves when it runs:internal_err!("Ordering direction required for DISTINCT with limit")aggr.group_expr().expr()[0]panics0on a top-k aggregateworst_val().expect("Missing root")panics: a queue of capacity 0 reports itself full with an empty rootFILTERGroupedTopKAggregateStreamnever readsfilter_expr, so the filter is silently dropped and the query returns wrong resultsCOUNT/AVG/… aggregatedebug_assert!in debug builds onlyNone of these are reachable from SQL: the optimizer rules that push a limit into an aggregate check the conditions first. They are reachable by any other rule that copies a limit onto a different node, by an external consumer of the physical plan API, and by decoding a plan from protobuf.
Describe the solution you'd like
Check the limit against the shape of the aggregate in
AggregateExecBuilder::build(added in #25376), which is the one place anAggregateExecis constructed.buildalready returns aResultand every caller already handles it: the two optimizer rules that push a limit down usebuild().ok()?, so a rejected limit means "skip this optimization" rather than a failed plan.Three shapes accept a limit:
SELECT DISTINCT-style aggregate: a group by, no aggregate expressions, noFILTER, and no ordering direction on the limit;MIN/MAXexpression, from the limit, or from both, and which also needs a limit above0, exactly one group by expression and no grouping sets, noFILTER, and a group key and value type the top-k queue supports.Describe alternatives you've considered
Writing the check as a list of the failure modes above is what #25376 first did, and it took three rounds of review to find the cases the list was missing in both directions — too loose on
COUNT, too strict on a planexecute_typedwould have run. The check should be derived fromAggregateExec::execute_typed's own dispatch, ideally sharing code with it, so the two cannot drift: rejecting a limit the executor would have run turns a working query into a planning error.It should also be tested by building each shape, executing it, and comparing against the same plan without the limit. The full sqllogictest suite passes with no check at all, because the optimizer never produces the plans the check exists to reject, so it carries no signal here.
A narrower alternative is to make two of these unrepresentable instead of merely rejected, by splitting
LimitOptionsintoSoftLimit { limit }andTopK { limit, descending }. That is worth doing as well, and is a wider rename.Additional context
Carved out of #25376, which is a pure refactor and does not change behaviour. Related: #25257.