Conversation
…ag when it is a no-op
Nothing recorded whether an aggregate cares about duplicates in its input.
`min(DISTINCT x)` already built a plain `MinAccumulator`, so the flag was a
no-op at execution time, but `SingleDistinctToGroupBy` still saw it and
rewrote `SELECT g, min(DISTINCT x) FROM t GROUP BY g` into an inner group by
at `(g, x)` grain. Callers also had no way to ask the question: the rule
identified sum/min/max by lowercased string name.
Add `AggregateUDFImpl::distinct_handling`, returning a three-valued
`DistinctHandling`:
- `Ignored` - the merge is idempotent, so `DISTINCT` cannot change the
result and the planner may drop it
- `Honored` - the accumulator reads `is_distinct` and deduplicates
- `Unsupported` - the accumulator does not implement `DISTINCT`
`Honored` is the default, so external UDFs are unaffected. The property is
forwarded from both the `AggregateUDF` wrapper and `AliasedAggregateUDFImpl`.
Tag the built-ins. `Ignored`: min, max, bool_and, bool_or, approx_distinct,
and the AND/OR arms of the bitwise operation (XOR cancels duplicate pairs and
stays `Honored`). `Unsupported`: the aggregates that either reject `DISTINCT`
already or silently return the non-distinct answer. first_value, last_value
and any_value keep the default with a TODO, because whether they ignore
duplicates depends on `ORDER BY`.
Add `EliminateAggregateDistinct`, inserted immediately before
`SingleDistinctToGroupBy`. It returns `Transformed::no` for every node that is
not an `Aggregate`, and uses `NamePreserver` to keep the output schema intact,
so the plan reads `min(t.v) AS min(DISTINCT t.v)`.
`Unsupported` is declared but not yet enforced. Rejecting `corr(DISTINCT x, y)`
fixes a silent wrong answer but turns queries that are accepted today into
planning errors, which belongs in its own change.
Over 4,000,000 rows in 2,000 groups, `min(DISTINCT x)` needed a 192M pool
before this change and completes under 1M after it.
Part of apache#24929, tracked as apache#11686.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu
…gged functions Review of the previous commit turned up two problems. `EliminateAggregateDistinct` stripped a no-op `DISTINCT` from one aggregate without regard for the others on the same node, which changed which plans `SingleDistinctToGroupBy` rewrites. That rule keys off how many distinct aggregates a node has and whether they share an argument, so removing one can push a node either way: `sum(DISTINCT cast(x)), max(DISTINCT x)` newly qualified once the max lost its flag, gaining an inner group by it never had, and where a rewrite happened regardless a shared inner group key turned into a separate accumulator at that finer grain. Gate the rule on every `DISTINCT` in the node being `Ignored`. This is conservative - `min(DISTINCT x), count(DISTINCT y)` now keeps the min flag though no rewrite is possible either way - but it cannot regress a plan. `group_by.slt` returns to its original expectations. `stddev`, `stddev_pop`, `var_samp`, `var_pop` and `approx_median` were tagged `Unsupported` on the grounds that their accumulators return `not_impl_err!`. That path is only reached when `SingleDistinctToGroupBy` cannot fire. All five take a single argument, so the rewrite deduplicates the input for them and `f(DISTINCT x)` returns the right answer today, as `aggregate.slt:749` already asserts for `approx_median`. Leave them at the default `Honored` with a comment, since enforcing `Unsupported` later would have broken those queries. Verified the remaining `Unsupported` tags: `approx_percentile_cont`, `nth_value` and `covar_samp` take more than one argument, so no rewrite fires and they silently return the non-distinct answer, while `approx_percentile_cont_with_weight` errors. Also widen the `Honored` and `Unsupported` doc comments, which described only the accumulator and promised a planning-time rejection that does not exist yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu
Clear the `DISTINCT` flag in place instead of matching the expression twice and rebuilding `AggregateFunctionParams` field by field. Collect the distinct aggregates' `DistinctHandling` and check them in one expression rather than tracking two flags with an early stop, and restore the saved name through `Transformed::update_data`. The strip helper still checks `DistinctHandling::Ignored` itself rather than relying on the node-level gate: the gate only inspects `aggr_expr`, while `map_expressions` also visits `group_expr`. A plan built with `groupBy=[sum(DISTINCT c)], aggr=[min(DISTINCT b)]` would otherwise lose the `sum` flag; SQL cannot produce it, but `Aggregate::try_new` does not reject it. `keep_honored_distinct_in_group_expr` covers this. Drop `eliminate_distinct_from_max`, which duplicated the `min` test and is covered by the min+max test. No plan changes: all optimizer snapshots and the aggregates_simplify, group_by, single_distinct_to_groupby and explain sqllogictests pass unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
The comments left on these functions in 957c8d6 said the accumulator rejects `DISTINCT` but `SingleDistinctToGroupBy` always deduplicates the input first, so `f(DISTINCT x)` returns the right answer today. Neither half holds for all five. `stddev`, `stddev_pop` and `approx_median` do reject `DISTINCT`, but they only work when the rewrite applies. Beside an aggregate that blocks it, such as `avg`, the query errors: SELECT g, stddev(DISTINCT x), avg(y) FROM t GROUP BY g Error: This feature is not implemented: STDDEV_POP(DISTINCT) aggregations are not available `var_samp` and `var_pop` never needed the rewrite: `DistinctVarianceAccumulator` deduplicates the input whenever `is_distinct` is set. Reword those comments, and the `DistinctHandling::Honored` doc, which listed `var_samp` among the planner-deduplicated functions and presented the rewrite as unconditional. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
…Handling non_exhaustive `grouping` was tagged `Unsupported` on the grounds that its accumulator silently returns the non-distinct answer. It does neither: the accumulator is never built, because `ResolveGroupingFunction` replaces the call with a value derived from the grouping id, and that value depends only on which grouping set a row belongs to. `grouping(DISTINCT x)` already returns the same result as `grouping(x)`, so enforcing `Unsupported` later would have rejected a working query. Tag it `Ignored`. The UDAF guide said to return `Unsupported` whenever the accumulator does not implement deduplication, which would cover `stddev` and `approx_median`, both left at `Honored` because `SingleDistinctToGroupBy` deduplicates for them when it applies. State the full condition, matching the enum doc, and say where those functions belong. `DistinctHandling` is public and nothing outside the defining crate matches on it exhaustively, so mark it `#[non_exhaustive]` now, before a release makes adding a variant a breaking change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
github-actions
Bot
added
documentation
mkleen
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25288 +/- ##
========================================
Coverage 81.91% 81.91%
========================================
Files 1134 1135 +1
Lines 425637 426260 +623
Branches 425637 426260 +623
========================================
+ Hits 348654 349174 +520
- Misses 56302 56349 +47
- Partials 20681 20737 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mkleen
marked this pull request as ready for review
September 14, 2026 09:04
mkleen
changed the title
Contributor
Author
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
jayzhan211
approved these changes
Sep 14, 2026
jayzhan211
left a comment
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mkleen , overall LGTM, suggestion on the naming
| pub enum DistinctHandling { | ||
| /// The result is the same with or without `DISTINCT`, so the planner | ||
| /// is free to drop it. `min`, `max`, `bool_and`, `bit_or`, ... | ||
| Ignored, |
Contributor
There was a problem hiding this comment.
What do you think
Ignored -> Insensitive
Honored -> Sensitive
Contributor
Author
There was a problem hiding this comment.
Yes, this is a good idea.
adriangb
approved these changes
Sep 14, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the new PR!
One request that is not in the inline comments:
Add unit tests with custom UDAFs for the public API. No test overrides distinct_handling. The unit tests use only the built-in min, max, sum and bit_xor, and no test references DistinctHandling::Unsupported. Please add tests in datafusion/core/tests/user_defined/user_defined_aggregates.rs (or in the rule's own tests with a test UDAF) that cover: a UDAF that returns Ignored loses the flag and keeps its output name, with FILTER and ORDER BY carried over; a UDAF that returns Honored keeps the flag; a UDAF that returns Unsupported keeps the flag; an Ignored UDAF beside an Honored distinct on the same node keeps both flags; an input that is already aliased (.alias("m")) gets no second alias; and a UDAF registered through AggregateUDF::with_aliases delegates the tag. Please also add a sqllogictest case for approx_distinct(DISTINCT v). It is the largest plan change in the PR and the one Ignored tag with no test.
The inline comments cover the rest: two test cases, the doc comments in the rule, and the definitions of the DistinctHandling variants.
Follow-up tracking issues, not for this PR:
datafusion/spark:collect_setalways usesDistinctArrayAggAccumulator, so it meets theIgnoreddefinition, andcollect_listandtry_sumnever readis_distinct. None of them is tagged.datafusion/ffi:ForeignAggregateUDFdoes not forwarddistinct_handling, so every UDAF that crosses the FFI boundary staysHonored. Safe, but the optimization cannot reachdatafusion-pythonusers.
Contributor
Author
|
@adriangb @jayzhan211 Thanks a lot for the review! I am on it with the follow-ups. |
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
min/maxcase).Rationale for this change
SELECT g, min(DISTINCT x) FROM t GROUP BY gis planned as ifDISTINCTmattered.SingleDistinctToGroupByrewrites it into an inner group by at(g, x)grain just to deduplicatex, even thoughminreturns the same value with or without duplicates. On a 4M-row table with ~1M distinctxvalues, that extra stage makes the query up to 10x slower and reserves ~190 MB where the plainmin(x)needs less than 1 MB.The same applies to every aggregate whose result cannot change when duplicates are removed:
max,bool_and,bool_or,bit_and,bit_or,approx_distinct.Nothing in DataFusion could say which aggregates those are.
SingleDistinctToGroupByidentifiessum/min/maxby lowercased function name, and a user-defined aggregate had no way to declare the property at all.What changes are included in this PR?
A new
AggregateUDFImpl::distinct_handling()method returning a new enumDistinctHandling:InsensitiveDISTINCT.min,max,bool_and,bool_or,bit_and,bit_or,approx_distinct,groupingSensitive(default)DISTINCTis applied, either by the accumulator itself (count,sum,avg,var_samp,array_agg,bit_xor, ...) or bySingleDistinctToGroupBydeduplicating the input first (stddev,approx_median).Unsupportedf(DISTINCT ...)errors or silently returns the non-distinct answer. Declaration only for now; rejecting these at planning time is a follow-up.corr,covar_samp,covar_pop,regr_*,nth_value,approx_percentile_cont,approx_percentile_cont_with_weightfirst_value,last_valueandany_valuestay atHonoredwith a TODO: whetherDISTINCTis a no-op for them depends onORDER BY.A new logical optimizer rule,
EliminateAggregateDistinct, placed beforeSingleDistinctToGroupBy. It clears theDISTINCTflag onIgnoredaggregates and keeps the output column name with an alias:What is the testing strategy for this PR?
datafusion/optimizer/src/eliminate_aggregate_distinct.rs:min,min+max, a non-distinct aggregate beside the distinct one,FILTER, the negative casessumandbit_xor, a mixed node that must be left alone (on its own and followed bySingleDistinctToGroupBy), anHonoreddistinct aggregate in the group expressions, and a plan with noAggregate.aggregates_simplify.sltgets a new section. For eachIgnoredaggregate it checks theEXPLAINplan and the query results, over data with duplicates andNULLs. It also covers the negative cases (bit_xor,count), a mixed node, andFILTER.single_distinct_to_groupby.slthas an updated expectation formin(DISTINCT v).explain.sltlists the new rule.DISTINCT" section inadding-udfs.md, and the rule inoptimizer_rule_reference.md.Benchmark
Here are some results from a local benchmark. Every query has the shape
SELECT g, <aggregate> FROM t GROUP BY g, over 4M rows wherexhas about 1M distinct values andbhas two.Speedup (
maintime / branch time):Peak memory-pool reservation in MB (
main→ branch):Are there any user-facing changes?
AggregateUDFImpl::distinct_handling()with defaultDistinctHandling::Honored, so existing implementations compile and behave as before.LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.