adriangb opened a new issue, #25393:
URL: https://github.com/apache/datafusion/issues/25393
### 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
https://github.com/apache/datafusion/pull/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
https://github.com/apache/datafusion/pull/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 https://github.com/apache/datafusion/pull/25376, which is a
pure refactor and does not change behaviour. Related:
https://github.com/apache/datafusion/issues/25257.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
