Skip to content

Reject a limit an AggregateExec cannot execute when the plan is built #25393

Description

@adriangb

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions