Repository navigation
Commit 982fca6
authored
fix: keep aggregate limits when combining or rebuilding AggregateExec (#25983)
## Which issue does this PR close?
- No separate issue. Both problems are regressions from #25696.
## Rationale for this change
1. Some `GROUP BY ... LIMIT` queries fail to plan. With one target
partition:
```sql
SET datafusion.execution.target_partitions = 1;
CREATE TABLE t AS SELECT * FROM (VALUES (1, 10), (2, 20), (3, 30), (4,
40), (5, 50), (6, 60)) AS v(a, b);
SELECT b, a FROM t GROUP BY b, a LIMIT 2;
SELECT a % 3 FROM t GROUP BY a % 3 LIMIT 2;
```
both fail with:
```
DataFusion error: CombinePartialFinalAggregate
caused by
Internal error: The AggregateKind should stay either (a) General (b)
DistinctLimit introduced with the previous optimizer pass
`LimitedDistinctAggregation`, it's impossible to have other variant.
```
When the final aggregate's grouping differs from the partial's
(expression keys, or columns not at their output positions),
`LimitedDistinctAggregation` limits only the final aggregate.
`CombinePartialFinalAggregate` treats that pair as impossible and
returns an error.
2. On Parquet tables the TopK aggregate limit is dropped. `EXPLAIN
SELECT DISTINCT k FROM pt ORDER BY k LIMIT 10` keeps `lim=[10]` on the
aggregates for a memory table but not for a Parquet table, so the query
falls back to a full hash aggregation. MIN/MAX TopK aggregates (`ORDER
BY max(x) DESC NULLS LAST LIMIT n`) lose their limit the same way.
Post-optimization filter pushdown rebuilds the Parquet scan when it
accepts a dynamic filter, and rebuilding the aggregates above it
re-applies only the DISTINCT limit, not the TopK ones.
Before #25696 both places carried the limit over with
`with_limit_options(...limit_options())`.
## What changes are included in this PR?
- `CombinePartialFinalAggregate`: if the final aggregate has a DISTINCT
limit, re-apply it to the combined aggregate with
`try_optimize_distinct_soft_limit`, which checks eligibility again.
Otherwise keep the combined aggregate as built.
- `AggregateExec::replace_children`
(`ChildrenPropertiesMode::Recompute`): also re-apply `TopKMinMax` and
`TopKDistinct` through `try_optimize_topk`, using the stored limit,
direction and NULL placement.
## What is the testing strategy for this PR?
New sqllogictest cases. Each one fails without the fix and passes with
it:
- `aggregate.slt`: both queries above at `target_partitions = 1`. An
`EXPLAIN` checks the combined `mode=Single ... lim=[2]` aggregate, and a
row-count check keeps the results deterministic.
- `aggregates_topk.slt`: a Parquet-backed table. `EXPLAIN` checks that
`lim=[10]` stays on both aggregates for `SELECT DISTINCT k ... ORDER BY
k LIMIT 10`, and that `lim=[2]` stays for a MAX TopK query whose scan
gets a semi-join dynamic filter. Result checks are included.
## Are there any user-facing changes?
No API changes. The listed queries plan again, and TopK aggregation
applies again on Parquet scans that receive dynamic filters.1 parent e5fdc9d commit 982fca6
4 files changed
Lines changed: 173 additions & 11 deletions
File tree
- datafusion
- physical-optimizer/src
- physical-plan/src/aggregates
- sqllogictest/test_files
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | | - | |
25 | 24 | | |
26 | 25 | | |
27 | 26 | | |
| |||
103 | 102 | | |
104 | 103 | | |
105 | 104 | | |
106 | | - | |
| 105 | + | |
107 | 106 | | |
108 | 107 | | |
109 | 108 | | |
| |||
126 | 125 | | |
127 | 126 | | |
128 | 127 | | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
134 | 131 | | |
135 | 132 | | |
136 | 133 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2520 | 2520 | | |
2521 | 2521 | | |
2522 | 2522 | | |
2523 | | - | |
2524 | | - | |
2525 | | - | |
| 2523 | + | |
| 2524 | + | |
| 2525 | + | |
| 2526 | + | |
2526 | 2527 | | |
2527 | | - | |
| 2528 | + | |
| 2529 | + | |
| 2530 | + | |
| 2531 | + | |
| 2532 | + | |
| 2533 | + | |
| 2534 | + | |
| 2535 | + | |
| 2536 | + | |
| 2537 | + | |
| 2538 | + | |
| 2539 | + | |
| 2540 | + | |
| 2541 | + | |
| 2542 | + | |
| 2543 | + | |
| 2544 | + | |
| 2545 | + | |
| 2546 | + | |
| 2547 | + | |
| 2548 | + | |
| 2549 | + | |
| 2550 | + | |
| 2551 | + | |
| 2552 | + | |
| 2553 | + | |
| 2554 | + | |
2528 | 2555 | | |
2529 | 2556 | | |
2530 | 2557 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7619 | 7619 | | |
7620 | 7620 | | |
7621 | 7621 | | |
| 7622 | + | |
| 7623 | + | |
| 7624 | + | |
| 7625 | + | |
| 7626 | + | |
| 7627 | + | |
| 7628 | + | |
| 7629 | + | |
| 7630 | + | |
| 7631 | + | |
| 7632 | + | |
| 7633 | + | |
| 7634 | + | |
| 7635 | + | |
| 7636 | + | |
| 7637 | + | |
| 7638 | + | |
| 7639 | + | |
| 7640 | + | |
| 7641 | + | |
| 7642 | + | |
| 7643 | + | |
| 7644 | + | |
| 7645 | + | |
| 7646 | + | |
| 7647 | + | |
| 7648 | + | |
| 7649 | + | |
| 7650 | + | |
| 7651 | + | |
| 7652 | + | |
| 7653 | + | |
| 7654 | + | |
| 7655 | + | |
| 7656 | + | |
| 7657 | + | |
| 7658 | + | |
| 7659 | + | |
| 7660 | + | |
| 7661 | + | |
| 7662 | + | |
| 7663 | + | |
| 7664 | + | |
| 7665 | + | |
| 7666 | + | |
| 7667 | + | |
| 7668 | + | |
| 7669 | + | |
| 7670 | + | |
| 7671 | + | |
| 7672 | + | |
| 7673 | + | |
| 7674 | + | |
| 7675 | + | |
| 7676 | + | |
| 7677 | + | |
7622 | 7678 | | |
7623 | 7679 | | |
7624 | 7680 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
789 | 789 | | |
790 | 790 | | |
791 | 791 | | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
| 798 | + | |
| 799 | + | |
| 800 | + | |
| 801 | + | |
| 802 | + | |
| 803 | + | |
| 804 | + | |
| 805 | + | |
| 806 | + | |
| 807 | + | |
| 808 | + | |
| 809 | + | |
| 810 | + | |
| 811 | + | |
| 812 | + | |
| 813 | + | |
| 814 | + | |
| 815 | + | |
| 816 | + | |
| 817 | + | |
| 818 | + | |
| 819 | + | |
| 820 | + | |
| 821 | + | |
| 822 | + | |
| 823 | + | |
| 824 | + | |
| 825 | + | |
| 826 | + | |
| 827 | + | |
| 828 | + | |
| 829 | + | |
| 830 | + | |
| 831 | + | |
| 832 | + | |
| 833 | + | |
| 834 | + | |
| 835 | + | |
| 836 | + | |
| 837 | + | |
| 838 | + | |
| 839 | + | |
| 840 | + | |
| 841 | + | |
| 842 | + | |
| 843 | + | |
| 844 | + | |
| 845 | + | |
| 846 | + | |
| 847 | + | |
| 848 | + | |
| 849 | + | |
| 850 | + | |
| 851 | + | |
| 852 | + | |
| 853 | + | |
| 854 | + | |
| 855 | + | |
| 856 | + | |
| 857 | + | |
| 858 | + | |
| 859 | + | |
| 860 | + | |
| 861 | + | |
| 862 | + | |
| 863 | + | |
| 864 | + | |
| 865 | + | |
| 866 | + | |
| 867 | + | |
| 868 | + | |
| 869 | + | |
| 870 | + | |
| 871 | + | |
| 872 | + | |
| 873 | + | |
0 commit comments