From bcfe457c7370147c541f031c37e900b590cd6da7 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Sat, 12 Sep 2026 09:26:35 +0200 Subject: [PATCH 01/23] feat: teach AggregateUDFImpl how it treats DISTINCT, and strip the flag 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 #24929, tracked as #11686. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu --- .../core/src/optimizer_rule_reference.md | 55 ++-- datafusion/expr/src/lib.rs | 8 +- datafusion/expr/src/udaf.rs | 38 +++ .../functions-aggregate/src/any_value.rs | 5 + .../src/approx_distinct.rs | 6 + .../functions-aggregate/src/approx_median.rs | 6 + .../src/approx_percentile_cont.rs | 8 + .../src/approx_percentile_cont_with_weight.rs | 6 + .../functions-aggregate/src/bit_and_or_xor.rs | 13 + .../functions-aggregate/src/bool_and_or.rs | 11 + .../functions-aggregate/src/correlation.rs | 8 + .../functions-aggregate/src/covariance.rs | 15 + .../functions-aggregate/src/first_last.rs | 12 + .../functions-aggregate/src/grouping.rs | 8 + datafusion/functions-aggregate/src/min_max.rs | 11 + .../functions-aggregate/src/nth_value.rs | 8 + datafusion/functions-aggregate/src/regr.rs | 8 + datafusion/functions-aggregate/src/stddev.rs | 11 + .../functions-aggregate/src/variance.rs | 11 + .../src/eliminate_aggregate_distinct.rs | 285 ++++++++++++++++++ datafusion/optimizer/src/lib.rs | 1 + datafusion/optimizer/src/optimizer.rs | 2 + .../test_files/aggregates_simplify.slt | 220 ++++++++++++++ .../sqllogictest/test_files/explain.slt | 4 + .../sqllogictest/test_files/group_by.slt | 48 ++- .../test_files/single_distinct_to_groupby.slt | 8 +- .../functions/adding-udfs.md | 17 ++ 27 files changed, 771 insertions(+), 62 deletions(-) create mode 100644 datafusion/optimizer/src/eliminate_aggregate_distinct.rs diff --git a/datafusion/core/src/optimizer_rule_reference.md b/datafusion/core/src/optimizer_rule_reference.md index 6f65ee92e7c6d..249f2dea08e58 100644 --- a/datafusion/core/src/optimizer_rule_reference.md +++ b/datafusion/core/src/optimizer_rule_reference.md @@ -35,33 +35,34 @@ Rule order matters. The default pipeline may change between releases. ### Logical Optimizer Rules -| order | rule | summary | -| ----- | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| 1 | `rewrite_set_comparison` | Rewrites `ANY` and `ALL` set-comparison subqueries into `EXISTS`-based boolean expressions with correct SQL NULL semantics. | -| 2 | `optimize_unions` | Flattens nested unions and removes unions with a single input. | -| 3 | `unions_to_filter` | Merges `UNION DISTINCT` branches that share the same source into a single filtered branch with a disjunctive predicate. | -| 4 | `simplify_expressions` | Constant-folds and simplifies expressions while preserving output names. | -| 5 | `replace_distinct_aggregate` | Rewrites `DISTINCT` and `DISTINCT ON` operators into aggregate-based plans that later rules can optimize further. | -| 6 | `eliminate_join` | Replaces keyless inner joins with a literal `false` filter by an empty relation. | -| 7 | `decorrelate_predicate_subquery` | Converts eligible `IN` and `EXISTS` predicate subqueries into semi or anti joins. | -| 8 | `scalar_subquery_to_join` | Rewrites eligible scalar subqueries into joins and adds schema-preserving projections. | -| 9 | `decorrelate_lateral_join` | Rewrites eligible lateral joins into regular joins. | -| 10 | `extract_equijoin_predicate` | Splits join filters into equijoin keys and residual predicates. | -| 11 | `eliminate_duplicated_expr` | Removes duplicate expressions from projections, aggregates, and similar operators. | -| 12 | `eliminate_filter` | Drops always-true filters and replaces always-false or NULL filters with empty relations. | -| 13 | `eliminate_cross_join` | Uses filter predicates to replace cross joins with inner joins when join keys can be found. | -| 14 | `eliminate_limit` | Removes no-op limits and simplifies trivial limit shapes. | -| 15 | `propagate_empty_relation` | Pushes empty-relation knowledge upward so operators fed by no rows collapse early. | -| 16 | `filter_null_join_keys` | Adds `IS NOT NULL` filters to nullable equijoin keys that can never match. | -| 17 | `eliminate_outer_join` | Rewrites outer joins to inner joins when later filters reject the NULL-extended rows. | -| 18 | `push_down_limit` | Moves literal limits to scans/unions, merges limits, and pushes `Sort(fetch=N)` onto a preserved join side only when every sort key is from it; the outer Sort stays. | -| 19 | `push_down_filter` | Moves filters as early as possible through filter-commutative operators. | -| 20 | `single_distinct_aggregation_to_group_by` | Rewrites single-column `DISTINCT` aggregations into two-stage `GROUP BY` plans. | -| 21 | `eliminate_group_by_constant` | Removes constant or functionally redundant expressions from `GROUP BY`. | -| 22 | `common_sub_expression_eliminate` | Computes repeated subexpressions once and reuses the result. | -| 23 | `extract_leaf_expressions` | Pulls cheap leaf expressions closer to data sources so later pruning and filter rules can act earlier. | -| 24 | `push_down_leaf_projections` | Pushes the helper projections created by leaf extraction toward leaf inputs. | -| 25 | `optimize_projections` | Prunes unused columns and removes unnecessary logical projections. | +| order | rule | summary | +|-------| ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | +| 1 | `rewrite_set_comparison` | Rewrites `ANY` and `ALL` set-comparison subqueries into `EXISTS`-based boolean expressions with correct SQL NULL semantics. | +| 2 | `optimize_unions` | Flattens nested unions and removes unions with a single input. | +| 3 | `unions_to_filter` | Merges `UNION DISTINCT` branches that share the same source into a single filtered branch with a disjunctive predicate. | +| 4 | `simplify_expressions` | Constant-folds and simplifies expressions while preserving output names. | +| 5 | `replace_distinct_aggregate` | Rewrites `DISTINCT` and `DISTINCT ON` operators into aggregate-based plans that later rules can optimize further. | +| 6 | `eliminate_join` | Replaces keyless inner joins with a literal `false` filter by an empty relation. | +| 7 | `decorrelate_predicate_subquery` | Converts eligible `IN` and `EXISTS` predicate subqueries into semi or anti joins. | +| 8 | `scalar_subquery_to_join` | Rewrites eligible scalar subqueries into joins and adds schema-preserving projections. | +| 9 | `decorrelate_lateral_join` | Rewrites eligible lateral joins into regular joins. | +| 10 | `extract_equijoin_predicate` | Splits join filters into equijoin keys and residual predicates. | +| 11 | `eliminate_duplicated_expr` | Removes duplicate expressions from projections, aggregates, and similar operators. | +| 12 | `eliminate_filter` | Drops always-true filters and replaces always-false or NULL filters with empty relations. | +| 13 | `eliminate_cross_join` | Uses filter predicates to replace cross joins with inner joins when join keys can be found. | +| 14 | `eliminate_limit` | Removes no-op limits and simplifies trivial limit shapes. | +| 15 | `propagate_empty_relation` | Pushes empty-relation knowledge upward so operators fed by no rows collapse early. | +| 16 | `filter_null_join_keys` | Adds `IS NOT NULL` filters to nullable equijoin keys that can never match. | +| 17 | `eliminate_outer_join` | Rewrites outer joins to inner joins when later filters reject the NULL-extended rows. | +| 18 | `push_down_limit` | Moves literal limits closer to scans and unions and merges adjacent limits. | +| 19 | `push_down_filter` | Moves filters as early as possible through filter-commutative operators. | +| 20 | `eliminate_aggregate_distinct` | Drops the `DISTINCT` modifier from aggregates whose result cannot change, such as `min`, `max` and `bit_or`. | +| 21 | `single_distinct_aggregation_to_group_by` | Rewrites single-column `DISTINCT` aggregations into two-stage `GROUP BY` plans. | +| 22 | `eliminate_group_by_constant` | Removes constant or functionally redundant expressions from `GROUP BY`. | +| 23 | `common_sub_expression_eliminate` | Computes repeated subexpressions once and reuses the result. | +| 24 | `extract_leaf_expressions` | Pulls cheap leaf expressions closer to data sources so later pruning and filter rules can act earlier. | +| 25 | `push_down_leaf_projections` | Pushes the helper projections created by leaf extraction toward leaf inputs. | +| 26 | `optimize_projections` | Prunes unused columns and removes unnecessary logical projections. | ### Physical Optimizer Rules diff --git a/datafusion/expr/src/lib.rs b/datafusion/expr/src/lib.rs index a904422989942..649f3de2520ac 100644 --- a/datafusion/expr/src/lib.rs +++ b/datafusion/expr/src/lib.rs @@ -137,10 +137,10 @@ pub use partition_evaluator::PartitionEvaluator; pub use sqlparser; pub use table_source::{TableProviderFilterPushDown, TableSource, TableType}; pub use udaf::{ - AggregateUDF, AggregateUDFImpl, ReversedUDAF, SetMonotonicity, StatisticsArgs, - UdafDisplayNameBuilder, UdafHumanDisplayBuilder, UdafSchemaNameBuilder, - UdafWindowFunctionDisplayNameBuilder, UdafWindowFunctionSchemaNameBuilder, - udaf_default_return_field, + AggregateUDF, AggregateUDFImpl, DistinctHandling, ReversedUDAF, SetMonotonicity, + StatisticsArgs, UdafDisplayNameBuilder, UdafHumanDisplayBuilder, + UdafSchemaNameBuilder, UdafWindowFunctionDisplayNameBuilder, + UdafWindowFunctionSchemaNameBuilder, udaf_default_return_field, }; #[expect(deprecated)] pub use udaf::{ diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 458b39c969b6f..0e4141cae990d 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -360,6 +360,11 @@ impl AggregateUDF { self.inner.supports_within_group_clause() } + /// See [`AggregateUDFImpl::distinct_handling`] for more details. + pub fn distinct_handling(&self) -> DistinctHandling { + self.inner.distinct_handling() + } + /// Returns the documentation for this Aggregate UDF. /// /// Documentation can be accessed programmatically as well as @@ -940,6 +945,17 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { false } + /// How this function treats the `DISTINCT` modifier. + /// + /// Return [`DistinctHandling::Ignored`] for duplicate-insensitive + /// functions so that `f(DISTINCT x)` is planned as `f(x)`, and + /// [`DistinctHandling::Unsupported`] if the accumulator does not + /// read `is_distinct`, so that `DISTINCT` is rejected at planning + /// time rather than silently ignored. + fn distinct_handling(&self) -> DistinctHandling { + DistinctHandling::Honored + } + /// Returns the documentation for this Aggregate UDF. /// /// Documentation can be accessed programmatically as well as @@ -1687,6 +1703,10 @@ impl AggregateUDFImpl for AliasedAggregateUDFImpl { self.inner.set_monotonicity(data_type) } + fn distinct_handling(&self) -> DistinctHandling { + self.inner.distinct_handling() + } + fn documentation(&self) -> Option<&Documentation> { self.inner.documentation() } @@ -1713,6 +1733,24 @@ pub enum SetMonotonicity { NotMonotonic, } +/// How an aggregate function treats the `DISTINCT` modifier. +/// +/// Mathematically, `Ignored` means the function's merge operation is +/// idempotent (its state forms a semilattice): f(S ⊎ S) = f(S), so +/// removing duplicates from the input cannot change the result. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +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, + /// The accumulator honours `AccumulatorArgs::is_distinct` and + /// deduplicates its input. `count`, `sum`, `avg`, `array_agg`, ... + Honored, + /// The accumulator does not implement `DISTINCT`. Planning + /// `f(DISTINCT ...)` is an error. `corr`, `regr_*`, `nth_value`, ... + Unsupported, +} + #[cfg(test)] mod test { use crate::{AggregateUDF, AggregateUDFImpl}; diff --git a/datafusion/functions-aggregate/src/any_value.rs b/datafusion/functions-aggregate/src/any_value.rs index dc3bd23d806fc..a7bbdc7359c0e 100644 --- a/datafusion/functions-aggregate/src/any_value.rs +++ b/datafusion/functions-aggregate/src/any_value.rs @@ -122,4 +122,9 @@ impl AggregateUDFImpl for AnyValue { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + // TODO: this is arguably `DistinctHandling::Ignored` — the accumulator + // ignores `is_distinct` and returns an unspecified input value either + // way. Grouped with `first_value`/`last_value` and left at the default + // `Honored` until that family is settled together. } diff --git a/datafusion/functions-aggregate/src/approx_distinct.rs b/datafusion/functions-aggregate/src/approx_distinct.rs index d88fef3a9f5af..161cdb68ad72e 100644 --- a/datafusion/functions-aggregate/src/approx_distinct.rs +++ b/datafusion/functions-aggregate/src/approx_distinct.rs @@ -38,6 +38,7 @@ use datafusion_common::{ DataFusionError, Result, downcast_value, internal_datafusion_err, internal_err, not_impl_err, }; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -870,6 +871,11 @@ impl AggregateUDFImpl for ApproxDistinct { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Updating an HLL register with a value already seen is a no-op. + DistinctHandling::Ignored + } } fn is_fixed_domain_type(data_type: &DataType) -> bool { diff --git a/datafusion/functions-aggregate/src/approx_median.rs b/datafusion/functions-aggregate/src/approx_median.rs index 162dc224f2ccb..b4e736ace4adb 100644 --- a/datafusion/functions-aggregate/src/approx_median.rs +++ b/datafusion/functions-aggregate/src/approx_median.rs @@ -25,6 +25,7 @@ use std::fmt::Debug; use std::sync::Arc; use datafusion_common::{Result, not_impl_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -147,4 +148,9 @@ impl AggregateUDFImpl for ApproxMedian { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs index 4af3574d8bd74..5c8441aec092b 100644 --- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs +++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs @@ -31,6 +31,7 @@ use datafusion_common::{ DataFusionError, Result, ScalarValue, downcast_value, internal_err, not_impl_err, plan_err, }; +use datafusion_expr::DistinctHandling; use datafusion_expr::expr::{AggregateFunction, Sort}; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; @@ -324,6 +325,13 @@ impl AggregateUDFImpl for ApproxPercentileCont { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } #[derive(Debug)] diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs index 90b24dc3c678d..1a810a3c36685 100644 --- a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs +++ b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs @@ -26,6 +26,7 @@ use arrow::{array::ArrayRef, datatypes::DataType}; use datafusion_common::ScalarValue; use datafusion_common::types::{NativeType, logical_float64}; use datafusion_common::{Result, not_impl_err, plan_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::expr::{AggregateFunction, Sort}; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::{ @@ -281,6 +282,11 @@ impl AggregateUDFImpl for ApproxPercentileContWithWeight { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } #[derive(Debug)] diff --git a/datafusion/functions-aggregate/src/bit_and_or_xor.rs b/datafusion/functions-aggregate/src/bit_and_or_xor.rs index 92212b328ee7d..716018a472b69 100644 --- a/datafusion/functions-aggregate/src/bit_and_or_xor.rs +++ b/datafusion/functions-aggregate/src/bit_and_or_xor.rs @@ -31,6 +31,7 @@ use datafusion_common::hash_utils::RandomState; use datafusion_common::cast::as_list_array; use datafusion_common::{Result, ScalarValue, not_impl_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -318,6 +319,18 @@ impl AggregateUDFImpl for BitwiseOperation { fn documentation(&self) -> Option<&Documentation> { Some(self.documentation) } + + fn distinct_handling(&self) -> DistinctHandling { + match self.operation { + // Bitwise AND/OR are idempotent: duplicates cannot change the + // result, so building a per-group `HashSet` buys nothing. + BitwiseOperationType::And | BitwiseOperationType::Or => { + DistinctHandling::Ignored + } + // XOR cancels duplicate pairs, so `DISTINCT` is meaningful. + BitwiseOperationType::Xor => DistinctHandling::Honored, + } + } } struct BitAndAccumulator { diff --git a/datafusion/functions-aggregate/src/bool_and_or.rs b/datafusion/functions-aggregate/src/bool_and_or.rs index 33c9880e2786b..34449d63ef2cb 100644 --- a/datafusion/functions-aggregate/src/bool_and_or.rs +++ b/datafusion/functions-aggregate/src/bool_and_or.rs @@ -29,6 +29,7 @@ use arrow::datatypes::{DataType, FieldRef}; use datafusion_common::internal_err; use datafusion_common::{Result, ScalarValue}; use datafusion_common::{downcast_value, not_impl_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::{AggregateOrderSensitivity, format_state_name}; use datafusion_expr::{ @@ -183,6 +184,11 @@ impl AggregateUDFImpl for BoolAnd { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Boolean AND/OR are idempotent: duplicates cannot change the result. + DistinctHandling::Ignored + } } #[derive(Debug, Default)] @@ -313,6 +319,11 @@ impl AggregateUDFImpl for BoolOr { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Boolean AND/OR are idempotent: duplicates cannot change the result. + DistinctHandling::Ignored + } } #[derive(Debug, Default)] diff --git a/datafusion/functions-aggregate/src/correlation.rs b/datafusion/functions-aggregate/src/correlation.rs index 525f6801d2fb7..c3832f8de61cd 100644 --- a/datafusion/functions-aggregate/src/correlation.rs +++ b/datafusion/functions-aggregate/src/correlation.rs @@ -31,6 +31,7 @@ use arrow::{ array::ArrayRef, datatypes::{DataType, Field}, }; +use datafusion_expr::DistinctHandling; use datafusion_expr::{EmitTo, GroupSelection, GroupsAccumulator}; use datafusion_functions_aggregate_common::aggregate::groups_accumulator::accumulate::accumulate_multiple; use log::debug; @@ -145,6 +146,13 @@ impl AggregateUDFImpl for Correlation { debug!("GroupsAccumulator is created for aggregate function `corr(c1, c2)`"); Ok(Box::new(CorrelationGroupsAccumulator::new())) } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } /// An accumulator to compute correlation diff --git a/datafusion/functions-aggregate/src/covariance.rs b/datafusion/functions-aggregate/src/covariance.rs index 454d56f8ea577..46abc825175f1 100644 --- a/datafusion/functions-aggregate/src/covariance.rs +++ b/datafusion/functions-aggregate/src/covariance.rs @@ -21,6 +21,7 @@ use arrow::array::ArrayRef; use arrow::datatypes::{DataType, Field, FieldRef}; use datafusion_common::cast::{as_float64_array, as_uint64_array}; use datafusion_common::{Result, ScalarValue}; +use datafusion_expr::DistinctHandling; use datafusion_expr::{ Accumulator, AggregateUDFImpl, Documentation, Signature, Volatility, function::{AccumulatorArgs, StateFieldsArgs}, @@ -128,6 +129,13 @@ impl AggregateUDFImpl for CovarianceSample { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } #[user_doc( @@ -206,6 +214,13 @@ impl AggregateUDFImpl for CovariancePopulation { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } /// An accumulator to compute covariance diff --git a/datafusion/functions-aggregate/src/first_last.rs b/datafusion/functions-aggregate/src/first_last.rs index f2d6af5bcf4c1..7fd512ba007f3 100644 --- a/datafusion/functions-aggregate/src/first_last.rs +++ b/datafusion/functions-aggregate/src/first_last.rs @@ -380,6 +380,12 @@ impl AggregateUDFImpl for FirstValue { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + // TODO: whether this is `DistinctHandling::Ignored` depends on `ORDER BY`. + // `first_value(DISTINCT x ORDER BY y)` deduplicates `x` and leaves the `y` + // ordering meaningless, while `first_value(DISTINCT x ORDER BY x)` is just + // `min(x)`. Left at the default `Honored` until that is settled, even + // though the accumulator ignores `is_distinct` today. } struct FirstLastGroupsAccumulator { @@ -1294,6 +1300,12 @@ impl AggregateUDFImpl for LastValue { ) -> Result> { create_groups_accumulator(&args, false, self.is_input_pre_ordered, self.name()) } + + // TODO: whether this is `DistinctHandling::Ignored` depends on `ORDER BY`. + // `last_value(DISTINCT x ORDER BY y)` deduplicates `x` and leaves the `y` + // ordering meaningless, while `last_value(DISTINCT x ORDER BY x)` is just + // `max(x)`. Left at the default `Honored` until that is settled, even + // though the accumulator ignores `is_distinct` today. } /// This accumulator is used when there is no ordering specified for the diff --git a/datafusion/functions-aggregate/src/grouping.rs b/datafusion/functions-aggregate/src/grouping.rs index 720a63aab6884..d94e5fcf50084 100644 --- a/datafusion/functions-aggregate/src/grouping.rs +++ b/datafusion/functions-aggregate/src/grouping.rs @@ -20,6 +20,7 @@ use arrow::datatypes::Field; use arrow::datatypes::{DataType, FieldRef}; use datafusion_common::{Result, not_impl_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::AccumulatorArgs; use datafusion_expr::function::StateFieldsArgs; use datafusion_expr::utils::format_state_name; @@ -110,4 +111,11 @@ impl AggregateUDFImpl for Grouping { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } diff --git a/datafusion/functions-aggregate/src/min_max.rs b/datafusion/functions-aggregate/src/min_max.rs index 89a1e8114f5e4..5e11981bb8db3 100644 --- a/datafusion/functions-aggregate/src/min_max.rs +++ b/datafusion/functions-aggregate/src/min_max.rs @@ -45,6 +45,7 @@ use arrow::datatypes::{ use crate::min_max::min_max_bytes::MinMaxBytesAccumulator; use crate::min_max::min_max_struct::MinMaxStructAccumulator; use datafusion_common::ScalarValue; +use datafusion_expr::DistinctHandling; use datafusion_expr::{ Accumulator, AggregateUDFImpl, Documentation, SetMonotonicity, Signature, Volatility, function::AccumulatorArgs, @@ -399,6 +400,11 @@ impl AggregateUDFImpl for Max { // the same as new values are seen. SetMonotonicity::Increasing } + + fn distinct_handling(&self) -> DistinctHandling { + // `MAX` is idempotent: duplicates cannot change the maximum. + DistinctHandling::Ignored + } } #[derive(Debug)] @@ -694,6 +700,11 @@ impl AggregateUDFImpl for Min { // the same as new values are seen. SetMonotonicity::Decreasing } + + fn distinct_handling(&self) -> DistinctHandling { + // `MIN` is idempotent: duplicates cannot change the minimum. + DistinctHandling::Ignored + } } #[derive(Debug)] diff --git a/datafusion/functions-aggregate/src/nth_value.rs b/datafusion/functions-aggregate/src/nth_value.rs index 5e7f9c6c3186b..d3571bc925c24 100644 --- a/datafusion/functions-aggregate/src/nth_value.rs +++ b/datafusion/functions-aggregate/src/nth_value.rs @@ -30,6 +30,7 @@ use datafusion_common::utils::{SingleRowListArrayBuilder, get_row_at_idx}; use datafusion_common::{ Result, ScalarValue, assert_or_internal_err, exec_err, not_impl_err, }; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -189,6 +190,13 @@ impl AggregateUDFImpl for NthValueAgg { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } #[derive(Debug)] diff --git a/datafusion/functions-aggregate/src/regr.rs b/datafusion/functions-aggregate/src/regr.rs index 5b5a144fd2322..6d679aabea368 100644 --- a/datafusion/functions-aggregate/src/regr.rs +++ b/datafusion/functions-aggregate/src/regr.rs @@ -22,6 +22,7 @@ use arrow::{array::ArrayRef, datatypes::DataType, datatypes::Field}; use datafusion_common::cast::{as_float64_array, as_uint64_array}; use datafusion_common::{HashMap, Result, ScalarValue}; use datafusion_doc::aggregate_doc_sections::DOC_SECTION_STATISTICAL; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -513,6 +514,13 @@ impl AggregateUDFImpl for Regr { fn documentation(&self) -> Option<&Documentation> { self.regr_type.documentation() } + + fn distinct_handling(&self) -> DistinctHandling { + // Duplicate-sensitive, but the accumulator does not read + // `is_distinct` and today silently returns the non-distinct answer. + // The tag records the intent; enforcement is a follow-up change. + DistinctHandling::Unsupported + } } /// `RegrAccumulator` is used to compute linear regression aggregate functions diff --git a/datafusion/functions-aggregate/src/stddev.rs b/datafusion/functions-aggregate/src/stddev.rs index af7e849d9939c..4b2dfde415f9b 100644 --- a/datafusion/functions-aggregate/src/stddev.rs +++ b/datafusion/functions-aggregate/src/stddev.rs @@ -27,6 +27,7 @@ use arrow::datatypes::FieldRef; use arrow::{array::ArrayRef, datatypes::DataType, datatypes::Field}; use datafusion_common::ScalarValue; use datafusion_common::{Result, internal_err, not_impl_err}; +use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -140,6 +141,11 @@ impl AggregateUDFImpl for Stddev { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } make_udaf_expr_and_func!( @@ -240,6 +246,11 @@ impl AggregateUDFImpl for StddevPop { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } /// An accumulator to compute the average diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index 072d064f76bd4..ae7954abf5be8 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -26,6 +26,7 @@ use arrow::{ }; use datafusion_common::cast::{as_float64_array, as_uint64_array}; use datafusion_common::{Result, ScalarValue}; +use datafusion_expr::DistinctHandling; use datafusion_expr::{ Accumulator, AggregateUDFImpl, Documentation, GroupSelection, GroupsAccumulator, Signature, Volatility, @@ -150,6 +151,11 @@ impl AggregateUDFImpl for VarianceSample { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } #[user_doc( @@ -252,6 +258,11 @@ impl AggregateUDFImpl for VariancePopulation { fn documentation(&self) -> Option<&Documentation> { self.doc() } + + fn distinct_handling(&self) -> DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`. + DistinctHandling::Unsupported + } } /// An accumulator to compute variance diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs new file mode 100644 index 0000000000000..eb8a7328dab79 --- /dev/null +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -0,0 +1,285 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! [`EliminateAggregateDistinct`] drops the `DISTINCT` modifier from aggregate +//! functions that report [`DistinctHandling::Ignored`] + +use crate::optimizer::ApplyOrder; +use crate::{OptimizerConfig, OptimizerRule}; + +use datafusion_common::Result; +use datafusion_common::tree_node::{Transformed, TreeNode}; +use datafusion_expr::expr::{AggregateFunction, AggregateFunctionParams}; +use datafusion_expr::expr_rewriter::NamePreserver; +use datafusion_expr::{DistinctHandling, Expr, LogicalPlan}; + +/// Optimizer rule that removes a `DISTINCT` modifier that cannot change the +/// result of the aggregate it is attached to. +/// +/// `min`, `max`, `bool_and`, `bit_or` and friends have an idempotent merge, so +/// `min(DISTINCT x)` and `min(x)` return the same value. Removing the flag here +/// keeps [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] from +/// rewriting the plan into an inner group by that only exists to deduplicate. +/// +/// An aggregate states how it treats duplicates through +/// [`datafusion_expr::AggregateUDFImpl::distinct_handling`]. +/// +/// ```text +/// Aggregate: groupBy=[[g]], aggr=[[min(DISTINCT x)]] +/// ``` +/// +/// becomes +/// +/// ```text +/// Aggregate: groupBy=[[g]], aggr=[[min(x) AS "min(DISTINCT x)"]] +/// ``` +/// +/// The alias keeps the output schema unchanged so the parent projection still +/// resolves. +#[derive(Default, Debug)] +pub struct EliminateAggregateDistinct {} + +impl EliminateAggregateDistinct { + pub fn new() -> Self { + Self {} + } +} + +impl OptimizerRule for EliminateAggregateDistinct { + fn name(&self) -> &str { + "eliminate_aggregate_distinct" + } + + fn apply_order(&self) -> Option { + Some(ApplyOrder::BottomUp) + } + + fn supports_rewrite(&self) -> bool { + true + } + + fn rewrite( + &self, + plan: LogicalPlan, + _config: &dyn OptimizerConfig, + ) -> Result> { + // Aggregate expressions only appear on Aggregate nodes, so every other + // node is a cheap no-op. Window functions carry their own `distinct` + // flag and are out of scope. + if !matches!(plan, LogicalPlan::Aggregate(_)) { + return Ok(Transformed::no(plan)); + } + + // Dropping `DISTINCT` changes `Expr::schema_name`, and with it the + // output schema of the Aggregate, so restore the original name. + let name_preserver = NamePreserver::new(&plan); + plan.map_expressions(|expr| { + // The aggregate may sit under an alias that type coercion added, + // so walk the expression rather than matching only its root. + let saved_name = name_preserver.save(&expr); + let rewritten = expr.transform_down(strip_ignored_distinct)?; + if rewritten.transformed { + Ok(Transformed::yes(saved_name.restore(rewritten.data))) + } else { + Ok(Transformed::no(rewritten.data)) + } + }) + } +} + +/// Drops `DISTINCT` from `expr` if it is an aggregate that ignores duplicates. +/// +/// An idempotent merge is also commutative, so an `Ignored` function is +/// insensitive to input order and `order_by` needs no extra guard. `filter` is +/// applied before deduplication either way, so it is carried over untouched. +fn strip_ignored_distinct(expr: Expr) -> Result> { + let Expr::AggregateFunction(AggregateFunction { func, params }) = &expr else { + return Ok(Transformed::no(expr)); + }; + if !params.distinct || func.distinct_handling() != DistinctHandling::Ignored { + return Ok(Transformed::no(expr)); + } + + let Expr::AggregateFunction(AggregateFunction { func, params }) = expr else { + unreachable!("matched Expr::AggregateFunction above") + }; + // Destructured exhaustively so a new field cannot be dropped silently. + let AggregateFunctionParams { + args, + distinct: _, + filter, + order_by, + null_treatment, + } = params; + + Ok(Transformed::yes(Expr::AggregateFunction( + AggregateFunction { + func, + params: AggregateFunctionParams { + args, + distinct: false, + filter, + order_by, + null_treatment, + }, + }, + ))) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::OptimizerContext; + use crate::assert_optimized_plan_eq_snapshot; + use crate::test::*; + + use datafusion_expr::{ExprFunctionExt, LogicalPlanBuilder, col, lit}; + use datafusion_functions_aggregate::expr_fn::{bit_xor, max, min, sum}; + + use std::sync::Arc; + + macro_rules! assert_optimized_plan_equal { + ( + $plan:expr, + @ $expected:literal $(,)? + ) => {{ + let optimizer_ctx = OptimizerContext::new().with_max_passes(1); + let rules: Vec> = + vec![Arc::new(EliminateAggregateDistinct::new())]; + assert_optimized_plan_eq_snapshot!( + optimizer_ctx, + rules, + $plan, + @ $expected, + ) + }}; + } + + /// `min(DISTINCT b)` loses the flag but keeps its column name. + #[test] + fn eliminate_distinct_from_min() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate(vec![col("a")], vec![min(col("b")).distinct().build()?])? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) AS min(DISTINCT test.b)]] + TableScan: test + ") + } + + /// `max(DISTINCT b)` is the other half of the same accumulator family. + #[test] + fn eliminate_distinct_from_max() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate(vec![col("a")], vec![max(col("b")).distinct().build()?])? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[max(test.b) AS max(DISTINCT test.b)]] + TableScan: test + ") + } + + /// `sum` deduplicates for real, so the flag stays. + #[test] + fn keep_distinct_on_sum() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate(vec![col("a")], vec![sum(col("b")).distinct().build()?])? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[sum(DISTINCT test.b)]] + TableScan: test + ") + } + + /// XOR cancels duplicate pairs, unlike its `bit_and`/`bit_or` siblings. + #[test] + fn keep_distinct_on_bit_xor() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate(vec![col("a")], vec![bit_xor(col("b")).distinct().build()?])? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[bit_xor(DISTINCT test.b)]] + TableScan: test + ") + } + + /// A plan mixing the two strips only the duplicate-insensitive one. + #[test] + fn eliminate_distinct_from_min_only() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + min(col("b")).distinct().build()?, + sum(col("c")).distinct().build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) AS min(DISTINCT test.b), sum(DISTINCT test.c)]] + TableScan: test + ") + } + + /// `FILTER` is applied before deduplication, so it rides along untouched. + #[test] + fn eliminate_distinct_keeps_filter() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + min(col("b")) + .distinct() + .filter(col("c").gt(lit(0u32))) + .build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) FILTER (WHERE test.c > UInt32(0)) AS min(DISTINCT test.b) FILTER (WHERE test.c > UInt32(0))]] + TableScan: test + ") + } + + /// A plan with no Aggregate takes the no-op path. + #[test] + fn non_aggregate_plan_is_unchanged() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .filter(col("b").gt(lit(1u32)))? + .project(vec![col("a")])? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Projection: test.a + Filter: test.b > UInt32(1) + TableScan: test + ") + } +} diff --git a/datafusion/optimizer/src/lib.rs b/datafusion/optimizer/src/lib.rs index fbe7ad2f4d327..1f5fa191d60ca 100644 --- a/datafusion/optimizer/src/lib.rs +++ b/datafusion/optimizer/src/lib.rs @@ -43,6 +43,7 @@ pub mod common_subexpr_eliminate; pub mod decorrelate; pub mod decorrelate_lateral_join; pub mod decorrelate_predicate_subquery; +pub mod eliminate_aggregate_distinct; pub mod eliminate_cross_join; pub mod eliminate_duplicated_expr; pub mod eliminate_filter; diff --git a/datafusion/optimizer/src/optimizer.rs b/datafusion/optimizer/src/optimizer.rs index ca4a6688b50c5..49c59014fc110 100644 --- a/datafusion/optimizer/src/optimizer.rs +++ b/datafusion/optimizer/src/optimizer.rs @@ -44,6 +44,7 @@ use datafusion_expr::{ use crate::common_subexpr_eliminate::CommonSubexprEliminate; use crate::decorrelate_lateral_join::DecorrelateLateralJoin; use crate::decorrelate_predicate_subquery::DecorrelatePredicateSubquery; +use crate::eliminate_aggregate_distinct::EliminateAggregateDistinct; use crate::eliminate_cross_join::EliminateCrossJoin; use crate::eliminate_duplicated_expr::EliminateDuplicatedExpr; use crate::eliminate_filter::EliminateFilter; @@ -308,6 +309,7 @@ impl Optimizer { // Filters can't be pushed down past Limits, we should do PushDownFilter after PushDownLimit Arc::new(PushDownLimit::new()), Arc::new(PushDownFilter::new()), + Arc::new(EliminateAggregateDistinct::new()), Arc::new(SingleDistinctToGroupBy::new()), // The previous optimizations added expressions and projections, // that might benefit from the following rules diff --git a/datafusion/sqllogictest/test_files/aggregates_simplify.slt b/datafusion/sqllogictest/test_files/aggregates_simplify.slt index c4055d17396c5..3c849a27a43ee 100644 --- a/datafusion/sqllogictest/test_files/aggregates_simplify.slt +++ b/datafusion/sqllogictest/test_files/aggregates_simplify.slt @@ -356,3 +356,223 @@ DROP TABLE IF EXISTS tbl; statement ok DROP TABLE sum_simplify_t; + +####### +# EliminateAggregateDistinct: DISTINCT is dropped from duplicate-insensitive +# aggregates, so no inner group by is planned to deduplicate the input. +####### + +statement ok +CREATE TABLE distinct_simplify_t (g INT, v INT, b BOOLEAN) AS VALUES + (1, 3, true), + (1, 3, true), + (1, 5, false), + (2, 7, true), + (2, 7, true), + (2, NULL, NULL); + +# min: DISTINCT cannot change the minimum +query II +SELECT g, min(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 3 +2 7 + +query TT +EXPLAIN SELECT g, min(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(distinct_simplify_t.v) AS min(DISTINCT distinct_simplify_t.v)]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# max +query II +SELECT g, max(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 5 +2 7 + +query TT +EXPLAIN SELECT g, max(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[max(distinct_simplify_t.v) AS max(DISTINCT distinct_simplify_t.v)]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[max(distinct_simplify_t.v) as max(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[max(distinct_simplify_t.v) as max(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# bool_and +query IB +SELECT g, bool_and(DISTINCT b) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 false +2 true + +query TT +EXPLAIN SELECT g, bool_and(DISTINCT b) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[bool_and(distinct_simplify_t.b) AS bool_and(DISTINCT distinct_simplify_t.b)]] +02)--TableScan: distinct_simplify_t projection=[g, b] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[bool_and(distinct_simplify_t.b) as bool_and(DISTINCT distinct_simplify_t.b)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bool_and(distinct_simplify_t.b) as bool_and(DISTINCT distinct_simplify_t.b)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# bool_or +query IB +SELECT g, bool_or(DISTINCT b) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 true +2 true + +query TT +EXPLAIN SELECT g, bool_or(DISTINCT b) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[bool_or(distinct_simplify_t.b) AS bool_or(DISTINCT distinct_simplify_t.b)]] +02)--TableScan: distinct_simplify_t projection=[g, b] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[bool_or(distinct_simplify_t.b) as bool_or(DISTINCT distinct_simplify_t.b)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bool_or(distinct_simplify_t.b) as bool_or(DISTINCT distinct_simplify_t.b)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# bit_and +query II +SELECT g, bit_and(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 1 +2 7 + +query TT +EXPLAIN SELECT g, bit_and(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[bit_and(distinct_simplify_t.v) AS bit_and(DISTINCT distinct_simplify_t.v)]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[bit_and(distinct_simplify_t.v) as bit_and(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bit_and(distinct_simplify_t.v) as bit_and(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# bit_or +query II +SELECT g, bit_or(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 7 +2 7 + +query TT +EXPLAIN SELECT g, bit_or(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[bit_or(distinct_simplify_t.v) AS bit_or(DISTINCT distinct_simplify_t.v)]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[bit_or(distinct_simplify_t.v) as bit_or(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bit_or(distinct_simplify_t.v) as bit_or(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# Negative case: bit_xor cancels duplicate pairs, so DISTINCT is kept and +# SingleDistinctToGroupBy still rewrites the plan. +query II +SELECT g, bit_xor(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 6 +2 7 + +query TT +EXPLAIN SELECT g, bit_xor(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Projection: distinct_simplify_t.g, bit_xor(alias1) AS bit_xor(DISTINCT distinct_simplify_t.v) +02)--Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[bit_xor(alias1)]] +03)----Aggregate: groupBy=[[distinct_simplify_t.g, distinct_simplify_t.v AS alias1]], aggr=[[]] +04)------TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)ProjectionExec: expr=[g@0 as g, bit_xor(alias1)@1 as bit_xor(DISTINCT distinct_simplify_t.v)] +02)--AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[bit_xor(alias1)] +03)----RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=4 +04)------AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bit_xor(alias1)] +05)--------AggregateExec: mode=FinalPartitioned, gby=[g@0 as g, alias1@1 as alias1], aggr=[] +06)----------RepartitionExec: partitioning=Hash([g@0, alias1@1], 4), input_partitions=1 +07)------------AggregateExec: mode=Partial, gby=[g@0 as g, v@1 as alias1], aggr=[] +08)--------------DataSourceExec: partitions=1, partition_sizes=[1] + +# Negative case: count deduplicates for real +query II +SELECT g, count(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 2 +2 1 + +query TT +EXPLAIN SELECT g, count(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Projection: distinct_simplify_t.g, count(alias1) AS count(DISTINCT distinct_simplify_t.v) +02)--Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[count(alias1)]] +03)----Aggregate: groupBy=[[distinct_simplify_t.g, distinct_simplify_t.v AS alias1]], aggr=[[]] +04)------TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)ProjectionExec: expr=[g@0 as g, count(alias1)@1 as count(DISTINCT distinct_simplify_t.v)] +02)--AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[count(alias1)] +03)----RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=4 +04)------AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[count(alias1)] +05)--------AggregateExec: mode=FinalPartitioned, gby=[g@0 as g, alias1@1 as alias1], aggr=[] +06)----------RepartitionExec: partitioning=Hash([g@0, alias1@1], 4), input_partitions=1 +07)------------AggregateExec: mode=Partial, gby=[g@0 as g, v@1 as alias1], aggr=[] +08)--------------DataSourceExec: partitions=1, partition_sizes=[1] + +# Mixed: only the duplicate-insensitive aggregate loses its DISTINCT +query III +SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 3 8 +2 7 7 + +query TT +EXPLAIN SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(distinct_simplify_t.v) AS min(DISTINCT distinct_simplify_t.v), sum(DISTINCT CAST(distinct_simplify_t.v AS Int64))]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +# FILTER is applied before deduplication, so it survives the rewrite +query II +SELECT g, min(DISTINCT v) FILTER (WHERE v > 3) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 5 +2 7 + +query TT +EXPLAIN SELECT g, min(DISTINCT v) FILTER (WHERE v > 3) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int32(3)) AS min(DISTINCT distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int64(3))]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int32(3)) as min(DISTINCT distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int64(3))] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int32(3)) as min(DISTINCT distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int64(3))] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + +statement ok +DROP TABLE distinct_simplify_t; diff --git a/datafusion/sqllogictest/test_files/explain.slt b/datafusion/sqllogictest/test_files/explain.slt index b6837002086ad..e3490644068e1 100644 --- a/datafusion/sqllogictest/test_files/explain.slt +++ b/datafusion/sqllogictest/test_files/explain.slt @@ -195,6 +195,7 @@ logical_plan after filter_null_join_keys SAME TEXT AS ABOVE logical_plan after eliminate_outer_join SAME TEXT AS ABOVE logical_plan after push_down_limit SAME TEXT AS ABOVE logical_plan after push_down_filter SAME TEXT AS ABOVE +logical_plan after eliminate_aggregate_distinct SAME TEXT AS ABOVE logical_plan after single_distinct_aggregation_to_group_by SAME TEXT AS ABOVE logical_plan after eliminate_group_by_constant SAME TEXT AS ABOVE logical_plan after common_sub_expression_eliminate SAME TEXT AS ABOVE @@ -220,6 +221,7 @@ logical_plan after filter_null_join_keys SAME TEXT AS ABOVE logical_plan after eliminate_outer_join SAME TEXT AS ABOVE logical_plan after push_down_limit SAME TEXT AS ABOVE logical_plan after push_down_filter SAME TEXT AS ABOVE +logical_plan after eliminate_aggregate_distinct SAME TEXT AS ABOVE logical_plan after single_distinct_aggregation_to_group_by SAME TEXT AS ABOVE logical_plan after eliminate_group_by_constant SAME TEXT AS ABOVE logical_plan after common_sub_expression_eliminate SAME TEXT AS ABOVE @@ -574,6 +576,7 @@ logical_plan after filter_null_join_keys SAME TEXT AS ABOVE logical_plan after eliminate_outer_join SAME TEXT AS ABOVE logical_plan after push_down_limit SAME TEXT AS ABOVE logical_plan after push_down_filter SAME TEXT AS ABOVE +logical_plan after eliminate_aggregate_distinct SAME TEXT AS ABOVE logical_plan after single_distinct_aggregation_to_group_by SAME TEXT AS ABOVE logical_plan after eliminate_group_by_constant SAME TEXT AS ABOVE logical_plan after common_sub_expression_eliminate SAME TEXT AS ABOVE @@ -599,6 +602,7 @@ logical_plan after filter_null_join_keys SAME TEXT AS ABOVE logical_plan after eliminate_outer_join SAME TEXT AS ABOVE logical_plan after push_down_limit SAME TEXT AS ABOVE logical_plan after push_down_filter SAME TEXT AS ABOVE +logical_plan after eliminate_aggregate_distinct SAME TEXT AS ABOVE logical_plan after single_distinct_aggregation_to_group_by SAME TEXT AS ABOVE logical_plan after eliminate_group_by_constant SAME TEXT AS ABOVE logical_plan after common_sub_expression_eliminate SAME TEXT AS ABOVE diff --git a/datafusion/sqllogictest/test_files/group_by.slt b/datafusion/sqllogictest/test_files/group_by.slt index 942a6f3cc8988..a7535c8976b61 100644 --- a/datafusion/sqllogictest/test_files/group_by.slt +++ b/datafusion/sqllogictest/test_files/group_by.slt @@ -4294,32 +4294,30 @@ EXPLAIN SELECT SUM(DISTINCT CAST(x AS DOUBLE)), MAX(DISTINCT x) FROM t1 GROUP BY ---- logical_plan 01)Projection: sum(DISTINCT t1.x), max(DISTINCT t1.x) -02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT CAST(t1.x AS Float64)), max(DISTINCT t1.x)]] +02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT CAST(t1.x AS Float64)), max(t1.x) AS max(DISTINCT t1.x)]] 03)----TableScan: t1 projection=[x, y] physical_plan 01)ProjectionExec: expr=[sum(DISTINCT t1.x)@1 as sum(DISTINCT t1.x), max(DISTINCT t1.x)@2 as max(DISTINCT t1.x)] -02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(DISTINCT t1.x)] +02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(t1.x) as max(DISTINCT t1.x)] 03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=1 -04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(DISTINCT t1.x)] +04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(t1.x) as max(DISTINCT t1.x)] 05)--------DataSourceExec: partitions=1, partition_sizes=[1] query TT EXPLAIN SELECT SUM(DISTINCT CAST(x AS DOUBLE)), MAX(DISTINCT CAST(x AS DOUBLE)) FROM t1 GROUP BY y; ---- logical_plan -01)Projection: sum(alias1) AS sum(DISTINCT t1.x), max(alias1) AS max(DISTINCT t1.x) -02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(alias1), max(alias1)]] -03)----Aggregate: groupBy=[[t1.y, CAST(t1.x AS Float64) AS alias1]], aggr=[[]] +01)Projection: sum(DISTINCT t1.x), max(DISTINCT t1.x) +02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT __common_expr_1 AS t1.x), max(__common_expr_1) AS max(DISTINCT t1.x)]] +03)----Projection: CAST(t1.x AS Float64) AS __common_expr_1, t1.y 04)------TableScan: t1 projection=[x, y] physical_plan -01)ProjectionExec: expr=[sum(alias1)@1 as sum(DISTINCT t1.x), max(alias1)@2 as max(DISTINCT t1.x)] -02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(alias1), max(alias1)] -03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=8 -04)------AggregateExec: mode=Partial, gby=[y@0 as y], aggr=[sum(alias1), max(alias1)] -05)--------AggregateExec: mode=FinalPartitioned, gby=[y@0 as y, alias1@1 as alias1], aggr=[] -06)----------RepartitionExec: partitioning=Hash([y@0, alias1@1], 8), input_partitions=1 -07)------------AggregateExec: mode=Partial, gby=[y@1 as y, CAST(x@0 AS Float64) as alias1], aggr=[] -08)--------------DataSourceExec: partitions=1, partition_sizes=[1] +01)ProjectionExec: expr=[sum(DISTINCT t1.x)@1 as sum(DISTINCT t1.x), max(DISTINCT t1.x)@2 as max(DISTINCT t1.x)] +02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(__common_expr_1) as max(DISTINCT t1.x)] +03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=1 +04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(__common_expr_1) as max(DISTINCT t1.x)] +05)--------ProjectionExec: expr=[CAST(x@0 AS Float64) as __common_expr_1, y@1 as y] +06)----------DataSourceExec: partitions=1, partition_sizes=[1] # create an unbounded table that contains ordered timestamp. statement ok @@ -4504,22 +4502,16 @@ EXPLAIN SELECT c1, count(distinct c2), min(distinct c2), sum(c3), max(c4) FROM a ---- logical_plan 01)Sort: aggregate_test_100.c1 ASC NULLS LAST -02)--Projection: aggregate_test_100.c1, count(alias1) AS count(DISTINCT aggregate_test_100.c2), min(alias1) AS min(DISTINCT aggregate_test_100.c2), sum(alias2) AS sum(aggregate_test_100.c3), max(alias3) AS max(aggregate_test_100.c4) -03)----Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[count(alias1), min(alias1), sum(alias2), max(alias3)]] -04)------Aggregate: groupBy=[[aggregate_test_100.c1, aggregate_test_100.c2 AS alias1]], aggr=[[sum(CAST(aggregate_test_100.c3 AS Int64)) AS alias2, max(aggregate_test_100.c4) AS alias3]] -05)--------TableScan: aggregate_test_100 projection=[c1, c2, c3, c4] +02)--Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) AS min(DISTINCT aggregate_test_100.c2), sum(CAST(aggregate_test_100.c3 AS Int64)), max(aggregate_test_100.c4)]] +03)----TableScan: aggregate_test_100 projection=[c1, c2, c3, c4] physical_plan 01)SortPreservingMergeExec: [c1@0 ASC NULLS LAST] -02)--ProjectionExec: expr=[c1@0 as c1, count(alias1)@1 as count(DISTINCT aggregate_test_100.c2), min(alias1)@2 as min(DISTINCT aggregate_test_100.c2), sum(alias2)@3 as sum(aggregate_test_100.c3), max(alias3)@4 as max(aggregate_test_100.c4)] -03)----SortExec: expr=[c1@0 ASC NULLS LAST], preserve_partitioning=[true] -04)------AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[count(alias1), min(alias1), sum(alias2), max(alias3)] -05)--------RepartitionExec: partitioning=Hash([c1@0], 8), input_partitions=8 -06)----------AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[count(alias1), min(alias1), sum(alias2), max(alias3)] -07)------------AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1, alias1@1 as alias1], aggr=[sum(aggregate_test_100.c3) as alias2, max(aggregate_test_100.c4) as alias3] -08)--------------RepartitionExec: partitioning=Hash([c1@0, alias1@1], 8), input_partitions=8 -09)----------------AggregateExec: mode=Partial, gby=[c1@0 as c1, c2@1 as alias1], aggr=[sum(aggregate_test_100.c3) as alias2, max(aggregate_test_100.c4) as alias3] -10)------------------RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1 -11)--------------------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c2, c3, c4], file_type=csv, has_header=true +02)--SortExec: expr=[c1@0 ASC NULLS LAST], preserve_partitioning=[true] +03)----AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) as min(DISTINCT aggregate_test_100.c2), sum(aggregate_test_100.c3), max(aggregate_test_100.c4)] +04)------RepartitionExec: partitioning=Hash([c1@0], 8), input_partitions=8 +05)--------AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) as min(DISTINCT aggregate_test_100.c2), sum(aggregate_test_100.c3), max(aggregate_test_100.c4)] +06)----------RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1 +07)------------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c2, c3, c4], file_type=csv, has_header=true query II SELECT c2, count(distinct c3) FILTER (WHERE c1 != 'a') FROM aggregate_test_100 GROUP BY c2 ORDER BY c2; diff --git a/datafusion/sqllogictest/test_files/single_distinct_to_groupby.slt b/datafusion/sqllogictest/test_files/single_distinct_to_groupby.slt index 8b032536fd420..fddfc661b8cc5 100644 --- a/datafusion/sqllogictest/test_files/single_distinct_to_groupby.slt +++ b/datafusion/sqllogictest/test_files/single_distinct_to_groupby.slt @@ -124,14 +124,16 @@ logical_plan 02)--Aggregate: groupBy=[[t.g]], aggr=[[count(Int64(1)), sum(DISTINCT CAST(t.v AS Int64))]] 03)----TableScan: t projection=[g, v] -# `min(DISTINCT v)` is the same value as `min(v)`, so the unrewritten plan keeps -# one scalar per group while the rewrite would build a row per distinct pair +# `min(DISTINCT v)` is the same value as `min(v)`. EliminateAggregateDistinct +# drops the flag before this rule runs, so there is no distinct aggregate left +# to rewrite and the plan keeps one scalar per group instead of a row per +# distinct pair query TT EXPLAIN SELECT g, count(*) AS records, min(DISTINCT v) AS distinct_min_v FROM t GROUP BY g; ---- logical_plan 01)Projection: t.g, count(Int64(1)) AS count(*) AS records, min(DISTINCT t.v) AS distinct_min_v -02)--Aggregate: groupBy=[[t.g]], aggr=[[count(Int64(1)), min(DISTINCT t.v)]] +02)--Aggregate: groupBy=[[t.g]], aggr=[[count(Int64(1)), min(t.v) AS min(DISTINCT t.v)]] 03)----TableScan: t projection=[g, v] # The gate covers only the count. A plan that already qualified through sum, diff --git a/docs/source/library-user-guide/functions/adding-udfs.md b/docs/source/library-user-guide/functions/adding-udfs.md index c3a40557a006d..f2dce04bf65e1 100644 --- a/docs/source/library-user-guide/functions/adding-udfs.md +++ b/docs/source/library-user-guide/functions/adding-udfs.md @@ -1116,6 +1116,22 @@ impl Accumulator for GeometricMean { } ``` +### Declaring how an Aggregate UDF treats `DISTINCT` + +By default DataFusion assumes an aggregate honours the `DISTINCT` modifier, which means the accumulator is expected to +read `AccumulatorArgs::is_distinct` and deduplicate its input. Override +[`AggregateUDFImpl::distinct_handling`] when that is not what your function does: + +- Return `DistinctHandling::Ignored` when duplicates cannot change the result, that is, when merging a value the + accumulator has already seen is a no-op. `min`, `max`, `bool_and` and `bit_or` are all in this group. The optimizer + then plans `f(DISTINCT x)` as `f(x)`, which skips both the per-group hash set and the extra grouping stage that + `SingleDistinctToGroupBy` would otherwise introduce. +- Return `DistinctHandling::Unsupported` when the accumulator does not implement deduplication at all. Today this is + a declaration only; rejecting such queries at planning time is a follow-up change. +- Leave the default `DistinctHandling::Honored` otherwise. + +Getting this wrong changes query results, so only claim `Ignored` if your merge is genuinely idempotent. + ### Registering an Aggregate UDF To register a Aggregate UDF, you need to wrap the function implementation in a [`AggregateUDF`] struct and then register @@ -1370,6 +1386,7 @@ async fn main() -> Result<()> { [`aggregateudf`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.AggregateUDF.html [`create_udaf`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/fn.create_udaf.html +[`aggregateudfimpl::distinct_handling`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.AggregateUDFImpl.html#method.distinct_handling [`advanced_udaf.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/udf/advanced_udaf.rs ## Adding a Table UDF From 958b5b2c519f7c7622c617fd160cb0a0299ab278 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Sat, 12 Sep 2026 10:23:56 +0200 Subject: [PATCH 02/23] fix: keep DISTINCT on mixed aggregate nodes, and correct three mis-tagged 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 Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu --- datafusion/expr/src/udaf.rs | 23 ++-- .../functions-aggregate/src/approx_median.rs | 9 +- datafusion/functions-aggregate/src/stddev.rs | 17 ++- .../functions-aggregate/src/variance.rs | 17 ++- .../src/eliminate_aggregate_distinct.rs | 122 +++++++++++++++++- .../test_files/aggregates_simplify.slt | 9 +- .../sqllogictest/test_files/group_by.slt | 48 ++++--- 7 files changed, 185 insertions(+), 60 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 0e4141cae990d..39051dde705b9 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -948,10 +948,13 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { /// How this function treats the `DISTINCT` modifier. /// /// Return [`DistinctHandling::Ignored`] for duplicate-insensitive - /// functions so that `f(DISTINCT x)` is planned as `f(x)`, and - /// [`DistinctHandling::Unsupported`] if the accumulator does not - /// read `is_distinct`, so that `DISTINCT` is rejected at planning - /// time rather than silently ignored. + /// functions so that `f(DISTINCT x)` is planned as `f(x)`. + /// + /// Return [`DistinctHandling::Unsupported`] if the accumulator neither + /// reads `is_distinct` nor is reached only after the planner has already + /// deduplicated the input. Nothing reads this variant yet: rejecting such + /// queries at planning time, rather than silently returning the + /// non-distinct answer, is a follow-up change. fn distinct_handling(&self) -> DistinctHandling { DistinctHandling::Honored } @@ -1743,11 +1746,15 @@ 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, - /// The accumulator honours `AccumulatorArgs::is_distinct` and - /// deduplicates its input. `count`, `sum`, `avg`, `array_agg`, ... + /// `DISTINCT` is applied, so the planner must leave it alone. Either the + /// accumulator reads `AccumulatorArgs::is_distinct` and deduplicates its + /// input (`count`, `sum`, `avg`, `array_agg`, ...), or the planner does it + /// first by rewriting the aggregate into a group by (`stddev`, `var_samp`, + /// `approx_median`, ...). This is the default. Honored, - /// The accumulator does not implement `DISTINCT`. Planning - /// `f(DISTINCT ...)` is an error. `corr`, `regr_*`, `nth_value`, ... + /// The accumulator does not implement `DISTINCT` and nothing deduplicates + /// the input for it, so `f(DISTINCT ...)` either errors or silently + /// returns the non-distinct answer. `corr`, `regr_*`, `nth_value`, ... Unsupported, } diff --git a/datafusion/functions-aggregate/src/approx_median.rs b/datafusion/functions-aggregate/src/approx_median.rs index b4e736ace4adb..6d87a2dabe75f 100644 --- a/datafusion/functions-aggregate/src/approx_median.rs +++ b/datafusion/functions-aggregate/src/approx_median.rs @@ -25,7 +25,6 @@ use std::fmt::Debug; use std::sync::Arc; use datafusion_common::{Result, not_impl_err}; -use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -149,8 +148,8 @@ impl AggregateUDFImpl for ApproxMedian { self.doc() } - fn distinct_handling(&self) -> DistinctHandling { - // The accumulator rejects `DISTINCT` with `not_impl_err!`. - DistinctHandling::Unsupported - } + // Left at the default `Honored`. The accumulator rejects `DISTINCT` with + // `not_impl_err!`, but this takes a single argument, so + // `SingleDistinctToGroupBy` deduplicates the input before the accumulator + // ever sees it and `f(DISTINCT x)` returns the right answer today. } diff --git a/datafusion/functions-aggregate/src/stddev.rs b/datafusion/functions-aggregate/src/stddev.rs index 4b2dfde415f9b..706a90503af82 100644 --- a/datafusion/functions-aggregate/src/stddev.rs +++ b/datafusion/functions-aggregate/src/stddev.rs @@ -27,7 +27,6 @@ use arrow::datatypes::FieldRef; use arrow::{array::ArrayRef, datatypes::DataType, datatypes::Field}; use datafusion_common::ScalarValue; use datafusion_common::{Result, internal_err, not_impl_err}; -use datafusion_expr::DistinctHandling; use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; use datafusion_expr::utils::format_state_name; use datafusion_expr::{ @@ -142,10 +141,10 @@ impl AggregateUDFImpl for Stddev { self.doc() } - fn distinct_handling(&self) -> DistinctHandling { - // The accumulator rejects `DISTINCT` with `not_impl_err!`. - DistinctHandling::Unsupported - } + // Left at the default `Honored`. The accumulator rejects `DISTINCT` with + // `not_impl_err!`, but this takes a single argument, so + // `SingleDistinctToGroupBy` deduplicates the input before the accumulator + // ever sees it and `f(DISTINCT x)` returns the right answer today. } make_udaf_expr_and_func!( @@ -247,10 +246,10 @@ impl AggregateUDFImpl for StddevPop { self.doc() } - fn distinct_handling(&self) -> DistinctHandling { - // The accumulator rejects `DISTINCT` with `not_impl_err!`. - DistinctHandling::Unsupported - } + // Left at the default `Honored`. The accumulator rejects `DISTINCT` with + // `not_impl_err!`, but this takes a single argument, so + // `SingleDistinctToGroupBy` deduplicates the input before the accumulator + // ever sees it and `f(DISTINCT x)` returns the right answer today. } /// An accumulator to compute the average diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index ae7954abf5be8..422f08c5e4779 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -26,7 +26,6 @@ use arrow::{ }; use datafusion_common::cast::{as_float64_array, as_uint64_array}; use datafusion_common::{Result, ScalarValue}; -use datafusion_expr::DistinctHandling; use datafusion_expr::{ Accumulator, AggregateUDFImpl, Documentation, GroupSelection, GroupsAccumulator, Signature, Volatility, @@ -152,10 +151,10 @@ impl AggregateUDFImpl for VarianceSample { self.doc() } - fn distinct_handling(&self) -> DistinctHandling { - // The accumulator rejects `DISTINCT` with `not_impl_err!`. - DistinctHandling::Unsupported - } + // Left at the default `Honored`. The accumulator rejects `DISTINCT` with + // `not_impl_err!`, but this takes a single argument, so + // `SingleDistinctToGroupBy` deduplicates the input before the accumulator + // ever sees it and `f(DISTINCT x)` returns the right answer today. } #[user_doc( @@ -259,10 +258,10 @@ impl AggregateUDFImpl for VariancePopulation { self.doc() } - fn distinct_handling(&self) -> DistinctHandling { - // The accumulator rejects `DISTINCT` with `not_impl_err!`. - DistinctHandling::Unsupported - } + // Left at the default `Honored`. The accumulator rejects `DISTINCT` with + // `not_impl_err!`, but this takes a single argument, so + // `SingleDistinctToGroupBy` deduplicates the input before the accumulator + // ever sees it and `f(DISTINCT x)` returns the right answer today. } /// An accumulator to compute variance diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index eb8a7328dab79..5637a18d31d65 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -22,7 +22,7 @@ use crate::optimizer::ApplyOrder; use crate::{OptimizerConfig, OptimizerRule}; use datafusion_common::Result; -use datafusion_common::tree_node::{Transformed, TreeNode}; +use datafusion_common::tree_node::{Transformed, TreeNode, TreeNodeRecursion}; use datafusion_expr::expr::{AggregateFunction, AggregateFunctionParams}; use datafusion_expr::expr_rewriter::NamePreserver; use datafusion_expr::{DistinctHandling, Expr, LogicalPlan}; @@ -80,7 +80,10 @@ impl OptimizerRule for EliminateAggregateDistinct { // Aggregate expressions only appear on Aggregate nodes, so every other // node is a cheap no-op. Window functions carry their own `distinct` // flag and are out of scope. - if !matches!(plan, LogicalPlan::Aggregate(_)) { + let LogicalPlan::Aggregate(aggregate) = &plan else { + return Ok(Transformed::no(plan)); + }; + if !every_distinct_is_ignored(&aggregate.aggr_expr)? { return Ok(Transformed::no(plan)); } @@ -101,6 +104,43 @@ impl OptimizerRule for EliminateAggregateDistinct { } } +/// Whether this node has a `DISTINCT` to drop and every `DISTINCT` on it can go. +/// +/// Stripping only some of them would change which plans +/// [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] rewrites, +/// because that rule keys off how many distinct aggregates a node has and +/// whether they share one argument. Removing one can push a node either way: +/// it can newly qualify a node that has two distinct columns and now has one, +/// or, where a rewrite happens regardless, it can turn a shared inner group +/// key into a separate accumulator at that finer grain. Neither is the point +/// of this rule, so a node keeps every flag unless it can lose them all. +/// +/// This is conservative: `min(DISTINCT x), count(DISTINCT y)` keeps the `min` +/// flag even though no rewrite is possible either way. +fn every_distinct_is_ignored(aggr_expr: &[Expr]) -> Result { + let mut found_ignored = false; + for expr in aggr_expr { + let mut all_ignored = true; + expr.apply(|e| { + if let Expr::AggregateFunction(AggregateFunction { func, params }) = e + && params.distinct + { + if func.distinct_handling() == DistinctHandling::Ignored { + found_ignored = true; + } else { + all_ignored = false; + return Ok(TreeNodeRecursion::Stop); + } + } + Ok(TreeNodeRecursion::Continue) + })?; + if !all_ignored { + return Ok(false); + } + } + Ok(found_ignored) +} + /// Drops `DISTINCT` from `expr` if it is an aggregate that ignores duplicates. /// /// An idempotent merge is also commutative, so an `Ignored` function is @@ -147,6 +187,7 @@ mod tests { use crate::assert_optimized_plan_eq_snapshot; use crate::test::*; + use crate::single_distinct_to_groupby::SingleDistinctToGroupBy; use datafusion_expr::{ExprFunctionExt, LogicalPlanBuilder, col, lit}; use datafusion_functions_aggregate::expr_fn::{bit_xor, max, min, sum}; @@ -225,9 +266,10 @@ mod tests { ") } - /// A plan mixing the two strips only the duplicate-insensitive one. + /// A node keeps every flag when one of them has to stay, so that this rule + /// cannot change which plans `SingleDistinctToGroupBy` rewrites. #[test] - fn eliminate_distinct_from_min_only() -> Result<()> { + fn mixed_node_is_left_alone() -> Result<()> { let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) .aggregate( @@ -240,7 +282,44 @@ mod tests { .build()?; assert_optimized_plan_equal!(plan, @r" - Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) AS min(DISTINCT test.b), sum(DISTINCT test.c)]] + Aggregate: groupBy=[[test.a]], aggr=[[min(DISTINCT test.b), sum(DISTINCT test.c)]] + TableScan: test + ") + } + + /// Several duplicate-insensitive aggregates all lose the flag together. + #[test] + fn eliminate_distinct_from_every_ignored_aggregate() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + min(col("b")).distinct().build()?, + max(col("c")).distinct().build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) AS min(DISTINCT test.b), max(test.c) AS max(DISTINCT test.c)]] + TableScan: test + ") + } + + /// A non-distinct aggregate alongside is no obstacle. + #[test] + fn eliminate_distinct_beside_non_distinct_aggregate() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![min(col("b")).distinct().build()?, sum(col("c"))], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[min(test.b) AS min(DISTINCT test.b), sum(test.c)]] TableScan: test ") } @@ -267,6 +346,39 @@ mod tests { ") } + /// The conservative gate exists so `SingleDistinctToGroupBy` still sees the + /// plan exactly as it did before this rule was added. + #[test] + fn mixed_node_still_reaches_single_distinct_to_groupby() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + min(col("b")).distinct().build()?, + sum(col("b")).distinct().build()?, + ], + )? + .build()?; + + let optimizer_ctx = OptimizerContext::new().with_max_passes(1); + let rules: Vec> = vec![ + Arc::new(EliminateAggregateDistinct::new()), + Arc::new(SingleDistinctToGroupBy::new()), + ]; + assert_optimized_plan_eq_snapshot!( + optimizer_ctx, + rules, + plan, + @r" + Projection: test.a, min(alias1) AS min(DISTINCT test.b), sum(alias1) AS sum(DISTINCT test.b) + Aggregate: groupBy=[[test.a]], aggr=[[min(alias1), sum(alias1)]] + Aggregate: groupBy=[[test.a, test.b AS alias1]], aggr=[[]] + TableScan: test + ", + ) + } + /// A plan with no Aggregate takes the no-op path. #[test] fn non_aggregate_plan_is_unchanged() -> Result<()> { diff --git a/datafusion/sqllogictest/test_files/aggregates_simplify.slt b/datafusion/sqllogictest/test_files/aggregates_simplify.slt index 3c849a27a43ee..0908bf536c999 100644 --- a/datafusion/sqllogictest/test_files/aggregates_simplify.slt +++ b/datafusion/sqllogictest/test_files/aggregates_simplify.slt @@ -536,7 +536,8 @@ physical_plan 07)------------AggregateExec: mode=Partial, gby=[g@0 as g, v@1 as alias1], aggr=[] 08)--------------DataSourceExec: partitions=1, partition_sizes=[1] -# Mixed: only the duplicate-insensitive aggregate loses its DISTINCT +# Mixed: a node that still needs one DISTINCT keeps all of them, so this rule +# cannot change which plans SingleDistinctToGroupBy rewrites query III SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; ---- @@ -547,12 +548,12 @@ query TT EXPLAIN SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g; ---- logical_plan -01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(distinct_simplify_t.v) AS min(DISTINCT distinct_simplify_t.v), sum(DISTINCT CAST(distinct_simplify_t.v AS Int64))]] +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT CAST(distinct_simplify_t.v AS Int64))]] 02)--TableScan: distinct_simplify_t projection=[g, v] physical_plan -01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] 02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 -03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) as min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] 04)------DataSourceExec: partitions=1, partition_sizes=[1] # FILTER is applied before deduplication, so it survives the rewrite diff --git a/datafusion/sqllogictest/test_files/group_by.slt b/datafusion/sqllogictest/test_files/group_by.slt index a7535c8976b61..942a6f3cc8988 100644 --- a/datafusion/sqllogictest/test_files/group_by.slt +++ b/datafusion/sqllogictest/test_files/group_by.slt @@ -4294,30 +4294,32 @@ EXPLAIN SELECT SUM(DISTINCT CAST(x AS DOUBLE)), MAX(DISTINCT x) FROM t1 GROUP BY ---- logical_plan 01)Projection: sum(DISTINCT t1.x), max(DISTINCT t1.x) -02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT CAST(t1.x AS Float64)), max(t1.x) AS max(DISTINCT t1.x)]] +02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT CAST(t1.x AS Float64)), max(DISTINCT t1.x)]] 03)----TableScan: t1 projection=[x, y] physical_plan 01)ProjectionExec: expr=[sum(DISTINCT t1.x)@1 as sum(DISTINCT t1.x), max(DISTINCT t1.x)@2 as max(DISTINCT t1.x)] -02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(t1.x) as max(DISTINCT t1.x)] +02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(DISTINCT t1.x)] 03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=1 -04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(t1.x) as max(DISTINCT t1.x)] +04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(DISTINCT t1.x)] 05)--------DataSourceExec: partitions=1, partition_sizes=[1] query TT EXPLAIN SELECT SUM(DISTINCT CAST(x AS DOUBLE)), MAX(DISTINCT CAST(x AS DOUBLE)) FROM t1 GROUP BY y; ---- logical_plan -01)Projection: sum(DISTINCT t1.x), max(DISTINCT t1.x) -02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(DISTINCT __common_expr_1 AS t1.x), max(__common_expr_1) AS max(DISTINCT t1.x)]] -03)----Projection: CAST(t1.x AS Float64) AS __common_expr_1, t1.y +01)Projection: sum(alias1) AS sum(DISTINCT t1.x), max(alias1) AS max(DISTINCT t1.x) +02)--Aggregate: groupBy=[[t1.y]], aggr=[[sum(alias1), max(alias1)]] +03)----Aggregate: groupBy=[[t1.y, CAST(t1.x AS Float64) AS alias1]], aggr=[[]] 04)------TableScan: t1 projection=[x, y] physical_plan -01)ProjectionExec: expr=[sum(DISTINCT t1.x)@1 as sum(DISTINCT t1.x), max(DISTINCT t1.x)@2 as max(DISTINCT t1.x)] -02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(DISTINCT t1.x), max(__common_expr_1) as max(DISTINCT t1.x)] -03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=1 -04)------AggregateExec: mode=Partial, gby=[y@1 as y], aggr=[sum(DISTINCT t1.x), max(__common_expr_1) as max(DISTINCT t1.x)] -05)--------ProjectionExec: expr=[CAST(x@0 AS Float64) as __common_expr_1, y@1 as y] -06)----------DataSourceExec: partitions=1, partition_sizes=[1] +01)ProjectionExec: expr=[sum(alias1)@1 as sum(DISTINCT t1.x), max(alias1)@2 as max(DISTINCT t1.x)] +02)--AggregateExec: mode=FinalPartitioned, gby=[y@0 as y], aggr=[sum(alias1), max(alias1)] +03)----RepartitionExec: partitioning=Hash([y@0], 8), input_partitions=8 +04)------AggregateExec: mode=Partial, gby=[y@0 as y], aggr=[sum(alias1), max(alias1)] +05)--------AggregateExec: mode=FinalPartitioned, gby=[y@0 as y, alias1@1 as alias1], aggr=[] +06)----------RepartitionExec: partitioning=Hash([y@0, alias1@1], 8), input_partitions=1 +07)------------AggregateExec: mode=Partial, gby=[y@1 as y, CAST(x@0 AS Float64) as alias1], aggr=[] +08)--------------DataSourceExec: partitions=1, partition_sizes=[1] # create an unbounded table that contains ordered timestamp. statement ok @@ -4502,16 +4504,22 @@ EXPLAIN SELECT c1, count(distinct c2), min(distinct c2), sum(c3), max(c4) FROM a ---- logical_plan 01)Sort: aggregate_test_100.c1 ASC NULLS LAST -02)--Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) AS min(DISTINCT aggregate_test_100.c2), sum(CAST(aggregate_test_100.c3 AS Int64)), max(aggregate_test_100.c4)]] -03)----TableScan: aggregate_test_100 projection=[c1, c2, c3, c4] +02)--Projection: aggregate_test_100.c1, count(alias1) AS count(DISTINCT aggregate_test_100.c2), min(alias1) AS min(DISTINCT aggregate_test_100.c2), sum(alias2) AS sum(aggregate_test_100.c3), max(alias3) AS max(aggregate_test_100.c4) +03)----Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[count(alias1), min(alias1), sum(alias2), max(alias3)]] +04)------Aggregate: groupBy=[[aggregate_test_100.c1, aggregate_test_100.c2 AS alias1]], aggr=[[sum(CAST(aggregate_test_100.c3 AS Int64)) AS alias2, max(aggregate_test_100.c4) AS alias3]] +05)--------TableScan: aggregate_test_100 projection=[c1, c2, c3, c4] physical_plan 01)SortPreservingMergeExec: [c1@0 ASC NULLS LAST] -02)--SortExec: expr=[c1@0 ASC NULLS LAST], preserve_partitioning=[true] -03)----AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) as min(DISTINCT aggregate_test_100.c2), sum(aggregate_test_100.c3), max(aggregate_test_100.c4)] -04)------RepartitionExec: partitioning=Hash([c1@0], 8), input_partitions=8 -05)--------AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[count(DISTINCT aggregate_test_100.c2), min(aggregate_test_100.c2) as min(DISTINCT aggregate_test_100.c2), sum(aggregate_test_100.c3), max(aggregate_test_100.c4)] -06)----------RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1 -07)------------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c2, c3, c4], file_type=csv, has_header=true +02)--ProjectionExec: expr=[c1@0 as c1, count(alias1)@1 as count(DISTINCT aggregate_test_100.c2), min(alias1)@2 as min(DISTINCT aggregate_test_100.c2), sum(alias2)@3 as sum(aggregate_test_100.c3), max(alias3)@4 as max(aggregate_test_100.c4)] +03)----SortExec: expr=[c1@0 ASC NULLS LAST], preserve_partitioning=[true] +04)------AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[count(alias1), min(alias1), sum(alias2), max(alias3)] +05)--------RepartitionExec: partitioning=Hash([c1@0], 8), input_partitions=8 +06)----------AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[count(alias1), min(alias1), sum(alias2), max(alias3)] +07)------------AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1, alias1@1 as alias1], aggr=[sum(aggregate_test_100.c3) as alias2, max(aggregate_test_100.c4) as alias3] +08)--------------RepartitionExec: partitioning=Hash([c1@0, alias1@1], 8), input_partitions=8 +09)----------------AggregateExec: mode=Partial, gby=[c1@0 as c1, c2@1 as alias1], aggr=[sum(aggregate_test_100.c3) as alias2, max(aggregate_test_100.c4) as alias3] +10)------------------RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1 +11)--------------------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c2, c3, c4], file_type=csv, has_header=true query II SELECT c2, count(distinct c3) FILTER (WHERE c1 != 'a') FROM aggregate_test_100 GROUP BY c2 ORDER BY c2; From 6eb63654c960cc26652a9023dd68ceeb75711dc9 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Mon, 14 Sep 2026 00:49:57 +0200 Subject: [PATCH 03/23] refactor: simplify EliminateAggregateDistinct 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 Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB --- .../src/eliminate_aggregate_distinct.rs | 129 +++++++----------- 1 file changed, 50 insertions(+), 79 deletions(-) diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index 5637a18d31d65..c0c98c965228c 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -23,7 +23,7 @@ use crate::{OptimizerConfig, OptimizerRule}; use datafusion_common::Result; use datafusion_common::tree_node::{Transformed, TreeNode, TreeNodeRecursion}; -use datafusion_expr::expr::{AggregateFunction, AggregateFunctionParams}; +use datafusion_expr::expr::AggregateFunction; use datafusion_expr::expr_rewriter::NamePreserver; use datafusion_expr::{DistinctHandling, Expr, LogicalPlan}; @@ -83,101 +83,68 @@ impl OptimizerRule for EliminateAggregateDistinct { let LogicalPlan::Aggregate(aggregate) = &plan else { return Ok(Transformed::no(plan)); }; - if !every_distinct_is_ignored(&aggregate.aggr_expr)? { + if !can_strip_every_distinct(&aggregate.aggr_expr)? { return Ok(Transformed::no(plan)); } // Dropping `DISTINCT` changes `Expr::schema_name`, and with it the - // output schema of the Aggregate, so restore the original name. + // output schema of the Aggregate, so restore the original name. The + // aggregate may sit under an alias that type coercion added, so walk + // the expression rather than matching only its root. let name_preserver = NamePreserver::new(&plan); plan.map_expressions(|expr| { - // The aggregate may sit under an alias that type coercion added, - // so walk the expression rather than matching only its root. let saved_name = name_preserver.save(&expr); - let rewritten = expr.transform_down(strip_ignored_distinct)?; - if rewritten.transformed { - Ok(Transformed::yes(saved_name.restore(rewritten.data))) - } else { - Ok(Transformed::no(rewritten.data)) - } + expr.transform_down(strip_ignored_distinct) + .map(|t| t.update_data(|e| saved_name.restore(e))) }) } } -/// Whether this node has a `DISTINCT` to drop and every `DISTINCT` on it can go. +/// Whether the node has at least one `DISTINCT` and every one is `Ignored`. /// /// Stripping only some of them would change which plans /// [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] rewrites, -/// because that rule keys off how many distinct aggregates a node has and -/// whether they share one argument. Removing one can push a node either way: -/// it can newly qualify a node that has two distinct columns and now has one, -/// or, where a rewrite happens regardless, it can turn a shared inner group -/// key into a separate accumulator at that finer grain. Neither is the point -/// of this rule, so a node keeps every flag unless it can lose them all. -/// -/// This is conservative: `min(DISTINCT x), count(DISTINCT y)` keeps the `min` -/// flag even though no rewrite is possible either way. -fn every_distinct_is_ignored(aggr_expr: &[Expr]) -> Result { - let mut found_ignored = false; +/// since that rule keys off how many distinct aggregates a node has and +/// whether they share one argument. This is conservative: `min(DISTINCT x), +/// count(DISTINCT y)` keeps the `min` flag though no rewrite is possible. +fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { + let mut handlings = vec![]; for expr in aggr_expr { - let mut all_ignored = true; expr.apply(|e| { if let Expr::AggregateFunction(AggregateFunction { func, params }) = e && params.distinct { - if func.distinct_handling() == DistinctHandling::Ignored { - found_ignored = true; - } else { - all_ignored = false; - return Ok(TreeNodeRecursion::Stop); - } + handlings.push(func.distinct_handling()); } Ok(TreeNodeRecursion::Continue) })?; - if !all_ignored { - return Ok(false); - } } - Ok(found_ignored) + Ok( + !handlings.is_empty() + && handlings.iter().all(|h| *h == DistinctHandling::Ignored), + ) } /// Drops `DISTINCT` from `expr` if it is an aggregate that ignores duplicates. /// +/// The handling is checked again here rather than trusted to +/// [`can_strip_every_distinct`], which only inspects `aggr_expr`, while +/// `map_expressions` also visits the group expressions. +/// /// An idempotent merge is also commutative, so an `Ignored` function is /// insensitive to input order and `order_by` needs no extra guard. `filter` is /// applied before deduplication either way, so it is carried over untouched. fn strip_ignored_distinct(expr: Expr) -> Result> { - let Expr::AggregateFunction(AggregateFunction { func, params }) = &expr else { - return Ok(Transformed::no(expr)); - }; - if !params.distinct || func.distinct_handling() != DistinctHandling::Ignored { - return Ok(Transformed::no(expr)); - } - - let Expr::AggregateFunction(AggregateFunction { func, params }) = expr else { - unreachable!("matched Expr::AggregateFunction above") - }; - // Destructured exhaustively so a new field cannot be dropped silently. - let AggregateFunctionParams { - args, - distinct: _, - filter, - order_by, - null_treatment, - } = params; - - Ok(Transformed::yes(Expr::AggregateFunction( - AggregateFunction { - func, - params: AggregateFunctionParams { - args, - distinct: false, - filter, - order_by, - null_treatment, - }, - }, - ))) + Ok(match expr { + Expr::AggregateFunction(mut agg) + if agg.params.distinct + && agg.func.distinct_handling() == DistinctHandling::Ignored => + { + agg.params.distinct = false; + Transformed::yes(Expr::AggregateFunction(agg)) + } + _ => Transformed::no(expr), + }) } #[cfg(test)] @@ -224,20 +191,6 @@ mod tests { ") } - /// `max(DISTINCT b)` is the other half of the same accumulator family. - #[test] - fn eliminate_distinct_from_max() -> Result<()> { - let table_scan = test_table_scan()?; - let plan = LogicalPlanBuilder::from(table_scan) - .aggregate(vec![col("a")], vec![max(col("b")).distinct().build()?])? - .build()?; - - assert_optimized_plan_equal!(plan, @r" - Aggregate: groupBy=[[test.a]], aggr=[[max(test.b) AS max(DISTINCT test.b)]] - TableScan: test - ") - } - /// `sum` deduplicates for real, so the flag stays. #[test] fn keep_distinct_on_sum() -> Result<()> { @@ -324,6 +277,24 @@ mod tests { ") } + /// The gate only inspects `aggr_expr`, so a distinct aggregate that honors + /// the flag in the group expressions must keep it. + #[test] + fn keep_honored_distinct_in_group_expr() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![sum(col("c")).distinct().build()?], + vec![min(col("b")).distinct().build()?], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[sum(DISTINCT test.c)]], aggr=[[min(test.b) AS min(DISTINCT test.b)]] + TableScan: test + ") + } + /// `FILTER` is applied before deduplication, so it rides along untouched. #[test] fn eliminate_distinct_keeps_filter() -> Result<()> { From 596b3263c79d2c266f1b8d18f44377edaca058e4 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Mon, 14 Sep 2026 00:58:00 +0200 Subject: [PATCH 04/23] docs: correct how stddev, variance and approx_median handle DISTINCT The comments left on these functions in 957c8d667 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 Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB --- datafusion/expr/src/udaf.rs | 8 +++++--- .../functions-aggregate/src/approx_median.rs | 7 ++++--- datafusion/functions-aggregate/src/stddev.rs | 14 ++++++++------ datafusion/functions-aggregate/src/variance.rs | 12 ++++-------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 39051dde705b9..d497e15a4de5f 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -1748,9 +1748,11 @@ pub enum DistinctHandling { Ignored, /// `DISTINCT` is applied, so the planner must leave it alone. Either the /// accumulator reads `AccumulatorArgs::is_distinct` and deduplicates its - /// input (`count`, `sum`, `avg`, `array_agg`, ...), or the planner does it - /// first by rewriting the aggregate into a group by (`stddev`, `var_samp`, - /// `approx_median`, ...). This is the default. + /// input (`count`, `sum`, `avg`, `var_samp`, `array_agg`, ...), or it + /// rejects `DISTINCT` and relies on `SingleDistinctToGroupBy` to + /// deduplicate the input first (`stddev`, `approx_median`, ...). The latter + /// works only when that rewrite applies; otherwise the query errors. This + /// is the default. Honored, /// The accumulator does not implement `DISTINCT` and nothing deduplicates /// the input for it, so `f(DISTINCT ...)` either errors or silently diff --git a/datafusion/functions-aggregate/src/approx_median.rs b/datafusion/functions-aggregate/src/approx_median.rs index 6d87a2dabe75f..6f8927a18825b 100644 --- a/datafusion/functions-aggregate/src/approx_median.rs +++ b/datafusion/functions-aggregate/src/approx_median.rs @@ -149,7 +149,8 @@ impl AggregateUDFImpl for ApproxMedian { } // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, but this takes a single argument, so - // `SingleDistinctToGroupBy` deduplicates the input before the accumulator - // ever sees it and `f(DISTINCT x)` returns the right answer today. + // `not_impl_err!`, so `f(DISTINCT x)` only works when + // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input + // first. When it cannot, for example beside an `avg`, the query errors + // rather than returning the non-distinct answer. } diff --git a/datafusion/functions-aggregate/src/stddev.rs b/datafusion/functions-aggregate/src/stddev.rs index 706a90503af82..9d83cea95d6b2 100644 --- a/datafusion/functions-aggregate/src/stddev.rs +++ b/datafusion/functions-aggregate/src/stddev.rs @@ -142,9 +142,10 @@ impl AggregateUDFImpl for Stddev { } // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, but this takes a single argument, so - // `SingleDistinctToGroupBy` deduplicates the input before the accumulator - // ever sees it and `f(DISTINCT x)` returns the right answer today. + // `not_impl_err!`, so `f(DISTINCT x)` only works when + // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input + // first. When it cannot, for example beside an `avg`, the query errors + // rather than returning the non-distinct answer. } make_udaf_expr_and_func!( @@ -247,9 +248,10 @@ impl AggregateUDFImpl for StddevPop { } // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, but this takes a single argument, so - // `SingleDistinctToGroupBy` deduplicates the input before the accumulator - // ever sees it and `f(DISTINCT x)` returns the right answer today. + // `not_impl_err!`, so `f(DISTINCT x)` only works when + // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input + // first. When it cannot, for example beside an `avg`, the query errors + // rather than returning the non-distinct answer. } /// An accumulator to compute the average diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index 422f08c5e4779..3c4003f099c15 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -151,10 +151,8 @@ impl AggregateUDFImpl for VarianceSample { self.doc() } - // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, but this takes a single argument, so - // `SingleDistinctToGroupBy` deduplicates the input before the accumulator - // ever sees it and `f(DISTINCT x)` returns the right answer today. + // Left at the default `Honored`: `DistinctVarianceAccumulator` + // deduplicates the input when `is_distinct` is set. } #[user_doc( @@ -258,10 +256,8 @@ impl AggregateUDFImpl for VariancePopulation { self.doc() } - // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, but this takes a single argument, so - // `SingleDistinctToGroupBy` deduplicates the input before the accumulator - // ever sees it and `f(DISTINCT x)` returns the right answer today. + // Left at the default `Honored`: `DistinctVarianceAccumulator` + // deduplicates the input when `is_distinct` is set. } /// An accumulator to compute variance From 145ff673a1c37bf00f94877d8923e3c9dfea114b Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Mon, 14 Sep 2026 01:04:06 +0200 Subject: [PATCH 05/23] fix: tag grouping as Ignored, align the UDAF guide, and mark DistinctHandling 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 Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB --- datafusion/expr/src/udaf.rs | 1 + datafusion/functions-aggregate/src/grouping.rs | 9 +++++---- docs/source/library-user-guide/functions/adding-udfs.md | 9 ++++++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index d497e15a4de5f..65a172471774b 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -1742,6 +1742,7 @@ pub enum SetMonotonicity { /// idempotent (its state forms a semilattice): f(S ⊎ S) = f(S), so /// removing duplicates from the input cannot change the result. #[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] 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`, ... diff --git a/datafusion/functions-aggregate/src/grouping.rs b/datafusion/functions-aggregate/src/grouping.rs index d94e5fcf50084..d6df35a7c854a 100644 --- a/datafusion/functions-aggregate/src/grouping.rs +++ b/datafusion/functions-aggregate/src/grouping.rs @@ -113,9 +113,10 @@ impl AggregateUDFImpl for Grouping { } fn distinct_handling(&self) -> DistinctHandling { - // Duplicate-sensitive, but the accumulator does not read - // `is_distinct` and today silently returns the non-distinct answer. - // The tag records the intent; enforcement is a follow-up change. - DistinctHandling::Unsupported + // The result depends only on which grouping set a row belongs to, not + // on how many rows share a value, so duplicates cannot change it. + // `ResolveGroupingFunction` replaces the call before execution either + // way, which is why the accumulator above is never built. + DistinctHandling::Ignored } } diff --git a/docs/source/library-user-guide/functions/adding-udfs.md b/docs/source/library-user-guide/functions/adding-udfs.md index f2dce04bf65e1..d52909d7eb0cb 100644 --- a/docs/source/library-user-guide/functions/adding-udfs.md +++ b/docs/source/library-user-guide/functions/adding-udfs.md @@ -1126,9 +1126,12 @@ read `AccumulatorArgs::is_distinct` and deduplicate its input. Override accumulator has already seen is a no-op. `min`, `max`, `bool_and` and `bit_or` are all in this group. The optimizer then plans `f(DISTINCT x)` as `f(x)`, which skips both the per-group hash set and the extra grouping stage that `SingleDistinctToGroupBy` would otherwise introduce. -- Return `DistinctHandling::Unsupported` when the accumulator does not implement deduplication at all. Today this is - a declaration only; rejecting such queries at planning time is a follow-up change. -- Leave the default `DistinctHandling::Honored` otherwise. +- Return `DistinctHandling::Unsupported` when the result depends on duplicates, the accumulator does not deduplicate + its input, and nothing deduplicates it first, so `f(DISTINCT x)` errors or silently returns the non-distinct answer. + Today this is a declaration only; rejecting such queries at planning time is a follow-up change. +- Leave the default `DistinctHandling::Honored` otherwise. That includes an accumulator that rejects `DISTINCT` with an + error but relies on `SingleDistinctToGroupBy` to deduplicate the input first, as `stddev` does: the query works when + that rewrite applies and errors when it does not. Getting this wrong changes query results, so only claim `Ignored` if your merge is genuinely idempotent. From 81de522e7aba359f69a9910af05f9888f6a7b726 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:51:12 +0200 Subject: [PATCH 06/23] Update datafusion/optimizer/src/eliminate_aggregate_distinct.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/optimizer/src/eliminate_aggregate_distinct.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index c0c98c965228c..55b5650c9cb1b 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -131,9 +131,12 @@ fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { /// [`can_strip_every_distinct`], which only inspects `aggr_expr`, while /// `map_expressions` also visits the group expressions. /// -/// An idempotent merge is also commutative, so an `Ignored` function is -/// insensitive to input order and `order_by` needs no extra guard. `filter` is -/// applied before deduplication either way, so it is carried over untouched. +/// An idempotent merge is not always commutative: `first_value` is +/// idempotent but order-sensitive. The rule does not need commutativity. +/// `Ignored` means that the result does not change when duplicates are +/// removed, and stripping `DISTINCT` only stops that removal. `order_by` and +/// `filter` are carried over untouched, so the function sees the same rows in +/// the same order, plus the duplicates that it ignores. fn strip_ignored_distinct(expr: Expr) -> Result> { Ok(match expr { Expr::AggregateFunction(mut agg) From 343df1726c58236ffcdeabcfb3f03b8275536671 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:51:30 +0200 Subject: [PATCH 07/23] Update datafusion/optimizer/src/eliminate_aggregate_distinct.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- .../src/eliminate_aggregate_distinct.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index 55b5650c9cb1b..ab6b494cc4d11 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -108,21 +108,26 @@ impl OptimizerRule for EliminateAggregateDistinct { /// whether they share one argument. This is conservative: `min(DISTINCT x), /// count(DISTINCT y)` keeps the `min` flag though no rewrite is possible. fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { - let mut handlings = vec![]; + let mut found_distinct = false; + let mut all_ignored = true; for expr in aggr_expr { expr.apply(|e| { if let Expr::AggregateFunction(AggregateFunction { func, params }) = e && params.distinct { - handlings.push(func.distinct_handling()); + found_distinct = true; + if func.distinct_handling() != DistinctHandling::Ignored { + all_ignored = false; + return Ok(TreeNodeRecursion::Stop); + } } Ok(TreeNodeRecursion::Continue) })?; + if !all_ignored { + break; + } } - Ok( - !handlings.is_empty() - && handlings.iter().all(|h| *h == DistinctHandling::Ignored), - ) + Ok(found_distinct && all_ignored) } /// Drops `DISTINCT` from `expr` if it is an aggregate that ignores duplicates. From bd621309a6d6df045a6a06c0c59680d22c2c2cca Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:51:45 +0200 Subject: [PATCH 08/23] Update datafusion/functions-aggregate/src/stddev.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/stddev.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/datafusion/functions-aggregate/src/stddev.rs b/datafusion/functions-aggregate/src/stddev.rs index 9d83cea95d6b2..3b02d74d89505 100644 --- a/datafusion/functions-aggregate/src/stddev.rs +++ b/datafusion/functions-aggregate/src/stddev.rs @@ -141,11 +141,11 @@ impl AggregateUDFImpl for Stddev { self.doc() } - // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, so `f(DISTINCT x)` only works when - // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input - // first. When it cannot, for example beside an `avg`, the query errors - // rather than returning the non-distinct answer. + fn distinct_handling(&self) -> datafusion_expr::DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`, so the + // planner has to deduplicate the input first. + datafusion_expr::DistinctHandling::Unsupported + } } make_udaf_expr_and_func!( From 605f5cfc1eb242200218dfda98dd32c3baa946ab Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:51:57 +0200 Subject: [PATCH 09/23] Update datafusion/functions-aggregate/src/approx_median.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/approx_median.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/datafusion/functions-aggregate/src/approx_median.rs b/datafusion/functions-aggregate/src/approx_median.rs index 6f8927a18825b..ddaa883a3fd68 100644 --- a/datafusion/functions-aggregate/src/approx_median.rs +++ b/datafusion/functions-aggregate/src/approx_median.rs @@ -148,9 +148,9 @@ impl AggregateUDFImpl for ApproxMedian { self.doc() } - // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, so `f(DISTINCT x)` only works when - // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input - // first. When it cannot, for example beside an `avg`, the query errors - // rather than returning the non-distinct answer. + fn distinct_handling(&self) -> datafusion_expr::DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`, so the + // planner has to deduplicate the input first. + datafusion_expr::DistinctHandling::Unsupported + } } From 8852f7a85921d64270b0a20d6515dbb63d1f05c2 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:52:08 +0200 Subject: [PATCH 10/23] Update docs/source/library-user-guide/functions/adding-udfs.md Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- .../library-user-guide/functions/adding-udfs.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/source/library-user-guide/functions/adding-udfs.md b/docs/source/library-user-guide/functions/adding-udfs.md index d52909d7eb0cb..5aa490b0a4810 100644 --- a/docs/source/library-user-guide/functions/adding-udfs.md +++ b/docs/source/library-user-guide/functions/adding-udfs.md @@ -1126,12 +1126,11 @@ read `AccumulatorArgs::is_distinct` and deduplicate its input. Override accumulator has already seen is a no-op. `min`, `max`, `bool_and` and `bit_or` are all in this group. The optimizer then plans `f(DISTINCT x)` as `f(x)`, which skips both the per-group hash set and the extra grouping stage that `SingleDistinctToGroupBy` would otherwise introduce. -- Return `DistinctHandling::Unsupported` when the result depends on duplicates, the accumulator does not deduplicate - its input, and nothing deduplicates it first, so `f(DISTINCT x)` errors or silently returns the non-distinct answer. - Today this is a declaration only; rejecting such queries at planning time is a follow-up change. -- Leave the default `DistinctHandling::Honored` otherwise. That includes an accumulator that rejects `DISTINCT` with an - error but relies on `SingleDistinctToGroupBy` to deduplicate the input first, as `stddev` does: the query works when - that rewrite applies and errors when it does not. +- Return `DistinctHandling::Unsupported` when the accumulator does not implement `DISTINCT`: it does not read + `is_distinct`, or it rejects `DISTINCT` with an error. The planner must then deduplicate the input first or reject + the query. Today this is a declaration only; rejecting such queries at planning time is a follow-up change. +- Leave the default `DistinctHandling::Honored` when the accumulator reads `AccumulatorArgs::is_distinct` and + deduplicates its input itself. Getting this wrong changes query results, so only claim `Ignored` if your merge is genuinely idempotent. From 37c51c9e454fb95c8a56dc2d2b321f428a7a4a86 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:52:34 +0200 Subject: [PATCH 11/23] Update datafusion/sqllogictest/test_files/aggregates_simplify.slt Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- .../test_files/aggregates_simplify.slt | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/datafusion/sqllogictest/test_files/aggregates_simplify.slt b/datafusion/sqllogictest/test_files/aggregates_simplify.slt index 0908bf536c999..02c85817d82e5 100644 --- a/datafusion/sqllogictest/test_files/aggregates_simplify.slt +++ b/datafusion/sqllogictest/test_files/aggregates_simplify.slt @@ -537,24 +537,31 @@ physical_plan 08)--------------DataSourceExec: partitions=1, partition_sizes=[1] # Mixed: a node that still needs one DISTINCT keeps all of them, so this rule -# cannot change which plans SingleDistinctToGroupBy rewrites +# cannot change which plans SingleDistinctToGroupBy rewrites. `count` needs the +# rewrite, and the plan below shows that it still happens. query III -SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +SELECT g, min(DISTINCT v), count(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; ---- -1 3 8 -2 7 7 +1 3 2 +2 7 1 query TT -EXPLAIN SELECT g, min(DISTINCT v), sum(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +EXPLAIN SELECT g, min(DISTINCT v), count(DISTINCT v) FROM distinct_simplify_t GROUP BY g; ---- logical_plan -01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT CAST(distinct_simplify_t.v AS Int64))]] -02)--TableScan: distinct_simplify_t projection=[g, v] +01)Projection: distinct_simplify_t.g, min(alias1) AS min(DISTINCT distinct_simplify_t.v), count(alias1) AS count(DISTINCT distinct_simplify_t.v) +02)--Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[min(alias1), count(alias1)]] +03)----Aggregate: groupBy=[[distinct_simplify_t.g, distinct_simplify_t.v AS alias1]], aggr=[[]] +04)------TableScan: distinct_simplify_t projection=[g, v] physical_plan -01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] -02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 -03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(DISTINCT distinct_simplify_t.v), sum(DISTINCT distinct_simplify_t.v)] -04)------DataSourceExec: partitions=1, partition_sizes=[1] +01)ProjectionExec: expr=[g@0 as g, min(alias1)@1 as min(DISTINCT distinct_simplify_t.v), count(alias1)@2 as count(DISTINCT distinct_simplify_t.v)] +02)--AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[min(alias1), count(alias1)] +03)----RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=4 +04)------AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(alias1), count(alias1)] +05)--------AggregateExec: mode=FinalPartitioned, gby=[g@0 as g, alias1@1 as alias1], aggr=[] +06)----------RepartitionExec: partitioning=Hash([g@0, alias1@1], 4), input_partitions=1 +07)------------AggregateExec: mode=Partial, gby=[g@0 as g, v@1 as alias1], aggr=[] +08)--------------DataSourceExec: partitions=1, partition_sizes=[1] # FILTER is applied before deduplication, so it survives the rewrite query II From 7e527f0515e7e0818c0068834750e4d18e82572c Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:52:52 +0200 Subject: [PATCH 12/23] Update datafusion/expr/src/udaf.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/expr/src/udaf.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 65a172471774b..05fc55f8bb0b2 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -1747,17 +1747,15 @@ 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, - /// `DISTINCT` is applied, so the planner must leave it alone. Either the - /// accumulator reads `AccumulatorArgs::is_distinct` and deduplicates its - /// input (`count`, `sum`, `avg`, `var_samp`, `array_agg`, ...), or it - /// rejects `DISTINCT` and relies on `SingleDistinctToGroupBy` to - /// deduplicate the input first (`stddev`, `approx_median`, ...). The latter - /// works only when that rewrite applies; otherwise the query errors. This - /// is the default. + /// The accumulator reads `AccumulatorArgs::is_distinct` and deduplicates + /// its input, so the planner must leave the flag alone. `count`, `sum`, + /// `avg`, `var_samp`, `array_agg`, ... This is the default. Honored, - /// The accumulator does not implement `DISTINCT` and nothing deduplicates - /// the input for it, so `f(DISTINCT ...)` either errors or silently - /// returns the non-distinct answer. `corr`, `regr_*`, `nth_value`, ... + /// The accumulator does not implement `DISTINCT`: it does not read + /// `is_distinct`, or it rejects `DISTINCT` with an error. The planner has + /// to deduplicate the input first (today `SingleDistinctToGroupBy` does + /// that for single-argument functions) or reject the query. `stddev`, + /// `approx_median`, `corr`, `regr_*`, `nth_value`, ... Unsupported, } From 2269404193fb5b492f952b292077ef1a1e74e925 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:53:08 +0200 Subject: [PATCH 13/23] Update datafusion/functions-aggregate/src/stddev.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/stddev.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/datafusion/functions-aggregate/src/stddev.rs b/datafusion/functions-aggregate/src/stddev.rs index 3b02d74d89505..14a28fa809979 100644 --- a/datafusion/functions-aggregate/src/stddev.rs +++ b/datafusion/functions-aggregate/src/stddev.rs @@ -247,11 +247,11 @@ impl AggregateUDFImpl for StddevPop { self.doc() } - // Left at the default `Honored`. The accumulator rejects `DISTINCT` with - // `not_impl_err!`, so `f(DISTINCT x)` only works when - // `SingleDistinctToGroupBy` rewrites the node and deduplicates the input - // first. When it cannot, for example beside an `avg`, the query errors - // rather than returning the non-distinct answer. + fn distinct_handling(&self) -> datafusion_expr::DistinctHandling { + // The accumulator rejects `DISTINCT` with `not_impl_err!`, so the + // planner has to deduplicate the input first. + datafusion_expr::DistinctHandling::Unsupported + } } /// An accumulator to compute the average From de845305e3fb60170d82655cb42a6c279936df71 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:53:27 +0200 Subject: [PATCH 14/23] Update docs/source/library-user-guide/functions/adding-udfs.md Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- docs/source/library-user-guide/functions/adding-udfs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/library-user-guide/functions/adding-udfs.md b/docs/source/library-user-guide/functions/adding-udfs.md index 5aa490b0a4810..c330d3db9612d 100644 --- a/docs/source/library-user-guide/functions/adding-udfs.md +++ b/docs/source/library-user-guide/functions/adding-udfs.md @@ -1118,7 +1118,7 @@ impl Accumulator for GeometricMean { ### Declaring how an Aggregate UDF treats `DISTINCT` -By default DataFusion assumes an aggregate honours the `DISTINCT` modifier, which means the accumulator is expected to +By default DataFusion assumes an aggregate honors the `DISTINCT` modifier, which means the accumulator is expected to read `AccumulatorArgs::is_distinct` and deduplicate its input. Override [`AggregateUDFImpl::distinct_handling`] when that is not what your function does: From cb5ac84cc189dcca85e179606094be72c8e4db99 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:53:43 +0200 Subject: [PATCH 15/23] Update datafusion/functions-aggregate/src/bit_and_or_xor.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/bit_and_or_xor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/functions-aggregate/src/bit_and_or_xor.rs b/datafusion/functions-aggregate/src/bit_and_or_xor.rs index 716018a472b69..8c6b6fe1b8e38 100644 --- a/datafusion/functions-aggregate/src/bit_and_or_xor.rs +++ b/datafusion/functions-aggregate/src/bit_and_or_xor.rs @@ -323,7 +323,7 @@ impl AggregateUDFImpl for BitwiseOperation { fn distinct_handling(&self) -> DistinctHandling { match self.operation { // Bitwise AND/OR are idempotent: duplicates cannot change the - // result, so building a per-group `HashSet` buys nothing. + // result. Only XOR has a distinct accumulator. BitwiseOperationType::And | BitwiseOperationType::Or => { DistinctHandling::Ignored } From 8975e11f85053854bacc52b3d96e0ec22fda478b Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:54:01 +0200 Subject: [PATCH 16/23] Update datafusion/functions-aggregate/src/grouping.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/grouping.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/datafusion/functions-aggregate/src/grouping.rs b/datafusion/functions-aggregate/src/grouping.rs index d6df35a7c854a..1c35b2cd04a28 100644 --- a/datafusion/functions-aggregate/src/grouping.rs +++ b/datafusion/functions-aggregate/src/grouping.rs @@ -115,8 +115,9 @@ impl AggregateUDFImpl for Grouping { fn distinct_handling(&self) -> DistinctHandling { // The result depends only on which grouping set a row belongs to, not // on how many rows share a value, so duplicates cannot change it. - // `ResolveGroupingFunction` replaces the call before execution either - // way, which is why the accumulator above is never built. + // `ResolveGroupingFunction` replaces the call before the optimizer + // runs, so this tag is not reachable from SQL and the accumulator + // above is never built. DistinctHandling::Ignored } } From 826345e6febf808ca850c855a483e093f9d0a877 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:54:16 +0200 Subject: [PATCH 17/23] Update datafusion/functions-aggregate/src/first_last.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/functions-aggregate/src/first_last.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/functions-aggregate/src/first_last.rs b/datafusion/functions-aggregate/src/first_last.rs index 7fd512ba007f3..c2992d276f80f 100644 --- a/datafusion/functions-aggregate/src/first_last.rs +++ b/datafusion/functions-aggregate/src/first_last.rs @@ -1303,8 +1303,8 @@ impl AggregateUDFImpl for LastValue { // TODO: whether this is `DistinctHandling::Ignored` depends on `ORDER BY`. // `last_value(DISTINCT x ORDER BY y)` deduplicates `x` and leaves the `y` - // ordering meaningless, while `last_value(DISTINCT x ORDER BY x)` is just - // `max(x)`. Left at the default `Honored` until that is settled, even + // ordering meaningless, while `last_value(DISTINCT x ORDER BY x)` is + // `max(x)` when `x` has no NULL. Left at the default `Honored` until that is settled, even // though the accumulator ignores `is_distinct` today. } From 569fa410ffd8cfaa493be57b06159bbd8369a755 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:56:30 +0200 Subject: [PATCH 18/23] Update datafusion/sqllogictest/test_files/aggregates_simplify.slt Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- .../test_files/aggregates_simplify.slt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/datafusion/sqllogictest/test_files/aggregates_simplify.slt b/datafusion/sqllogictest/test_files/aggregates_simplify.slt index 02c85817d82e5..a97135470caad 100644 --- a/datafusion/sqllogictest/test_files/aggregates_simplify.slt +++ b/datafusion/sqllogictest/test_files/aggregates_simplify.slt @@ -582,5 +582,20 @@ physical_plan 03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[min(distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int32(3)) as min(DISTINCT distinct_simplify_t.v) FILTER (WHERE distinct_simplify_t.v > Int64(3))] 04)------DataSourceExec: partitions=1, partition_sizes=[1] +# min/max(DISTINCT) over floats now sees the raw values. Before this rule, the +# inner group by of SingleDistinctToGroupBy turned -0.0 into 0.0 in its hash +# key, so max(DISTINCT v) printed 0.0 here. Now it agrees with max(v). +# The R column type prints -0.0 as 0, so compare the text form. +statement ok +CREATE TABLE distinct_simplify_float_t (v DOUBLE) AS VALUES (-1.0), (-0.0), (-0.0); + +query TTTT +SELECT cast(min(DISTINCT v) AS VARCHAR), cast(max(DISTINCT v) AS VARCHAR), cast(min(v) AS VARCHAR), cast(max(v) AS VARCHAR) FROM distinct_simplify_float_t; +---- +-1.0 -0.0 -1.0 -0.0 + +statement ok +DROP TABLE distinct_simplify_float_t; + statement ok DROP TABLE distinct_simplify_t; From 5807127bebd5ed48c2d63bbfabc132a5f9e12415 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:56:59 +0200 Subject: [PATCH 19/23] Update datafusion/expr/src/udaf.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- datafusion/expr/src/udaf.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 05fc55f8bb0b2..d28e697d309c9 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -950,11 +950,11 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { /// Return [`DistinctHandling::Ignored`] for duplicate-insensitive /// functions so that `f(DISTINCT x)` is planned as `f(x)`. /// - /// Return [`DistinctHandling::Unsupported`] if the accumulator neither - /// reads `is_distinct` nor is reached only after the planner has already - /// deduplicated the input. Nothing reads this variant yet: rejecting such - /// queries at planning time, rather than silently returning the - /// non-distinct answer, is a follow-up change. + /// Return [`DistinctHandling::Unsupported`] if the accumulator does not + /// implement `DISTINCT`, that is, it does not read `is_distinct`, or it + /// rejects `DISTINCT` with an error. The planner then has to deduplicate + /// the input or reject the query. Nothing reads this variant yet: + /// rejecting such queries at planning time is a follow-up change. fn distinct_handling(&self) -> DistinctHandling { DistinctHandling::Honored } From b13035077731cf7b73fe61953adcf9bf720f2dda Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Tue, 15 Sep 2026 09:57:12 +0200 Subject: [PATCH 20/23] Update datafusion/optimizer/src/eliminate_aggregate_distinct.rs Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> --- .../optimizer/src/eliminate_aggregate_distinct.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index ab6b494cc4d11..2af8bbfa7df12 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -102,11 +102,15 @@ impl OptimizerRule for EliminateAggregateDistinct { /// Whether the node has at least one `DISTINCT` and every one is `Ignored`. /// -/// Stripping only some of them would change which plans -/// [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] rewrites, -/// since that rule keys off how many distinct aggregates a node has and -/// whether they share one argument. This is conservative: `min(DISTINCT x), -/// count(DISTINCT y)` keeps the `min` flag though no rewrite is possible. +/// If only some of them were stripped, an `Honored` `DISTINCT` could stay +/// beside a stripped aggregate. A stripped aggregate carries an alias, and +/// [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] does not +/// rewrite a node whose `aggr_expr` contains an alias. So `min(DISTINCT x), +/// count(DISTINCT x)` would lose the rewrite that `count` needs. When every +/// `DISTINCT` is `Ignored`, none is left after stripping, so that rule has +/// nothing to rewrite. This is conservative: `min(DISTINCT x), +/// count(DISTINCT y)` keeps the `min` flag. That flag costs nothing at run +/// time, because `min` ignores `is_distinct` when it selects its accumulator. fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { let mut found_distinct = false; let mut all_ignored = true; From 0676abaea56f308d58cb855b427eeb758439b087 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Wed, 16 Sep 2026 10:07:44 +0200 Subject: [PATCH 21/23] test: cover distinct_handling through the public UDAF API The rule's tests read the tag off built-in functions, so they only covered the variants those functions carry, and nothing exercised an `AggregateUDFImpl::distinct_handling` override, the API a third-party function uses. Add a `TaggedUdaf` test helper that reports the handling it was built with, and tests that an `Ignored` UDAF loses the flag while keeping its output name, `FILTER` and `ORDER BY`; that `Honored` and `Unsupported` keep it; that an `Ignored` UDAF beside an `Honored` one keeps both; that an aggregate the caller already aliased gets no second alias; and that `AggregateUDF::with_aliases` passes the tag through. Also add a sqllogictest for `approx_distinct(DISTINCT v)`, the largest plan change the rule makes and the one `Ignored` tag with no test: the deduplicating inner group by that `SingleDistinctToGroupBy` planned here is gone, because setting an HLL register a value already reached is a no-op. Co-Authored-By: Claude Opus 5 --- .../src/eliminate_aggregate_distinct.rs | 213 +++++++++++++++++- .../test_files/aggregates_simplify.slt | 21 ++ 2 files changed, 233 insertions(+), 1 deletion(-) diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index 2af8bbfa7df12..a44af92871557 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -167,9 +167,15 @@ mod tests { use crate::test::*; use crate::single_distinct_to_groupby::SingleDistinctToGroupBy; - use datafusion_expr::{ExprFunctionExt, LogicalPlanBuilder, col, lit}; + use arrow::datatypes::DataType; + use datafusion_expr::function::AccumulatorArgs; + use datafusion_expr::{ + Accumulator, AggregateUDF, AggregateUDFImpl, ExprFunctionExt, LogicalPlanBuilder, + Signature, Volatility, col, lit, + }; use datafusion_functions_aggregate::expr_fn::{bit_xor, max, min, sum}; + use std::hash::{Hash, Hasher}; use std::sync::Arc; macro_rules! assert_optimized_plan_equal { @@ -189,6 +195,68 @@ mod tests { }}; } + /// A user defined aggregate that reports the [`DistinctHandling`] it was + /// built with. + /// + /// The tests above read the tag off built-in functions, which only covers + /// the variants those functions happen to carry. This one exercises the + /// public API a third-party function uses: an + /// [`AggregateUDFImpl::distinct_handling`] override. + #[derive(Debug, Clone, PartialEq, Eq)] + struct TaggedUdaf { + name: &'static str, + handling: DistinctHandling, + signature: Signature, + } + + impl TaggedUdaf { + fn new(name: &'static str, handling: DistinctHandling) -> Self { + Self { + name, + handling, + signature: Signature::any(1, Volatility::Immutable), + } + } + } + + /// Hashed by name, which identifies the function here. `DistinctHandling` + /// is not `Hash`, and `AggregateUDFImpl` requires one through `DynHash`. + impl Hash for TaggedUdaf { + fn hash(&self, state: &mut H) { + self.name.hash(state); + self.signature.hash(state); + } + } + + impl AggregateUDFImpl for TaggedUdaf { + fn name(&self) -> &str { + self.name + } + + fn signature(&self) -> &Signature { + &self.signature + } + + fn return_type(&self, _arg_types: &[DataType]) -> Result { + Ok(DataType::UInt32) + } + + fn accumulator( + &self, + _acc_args: AccumulatorArgs, + ) -> Result> { + unimplemented!("the rule only rewrites the logical plan") + } + + fn distinct_handling(&self) -> DistinctHandling { + self.handling + } + } + + fn tagged(name: &'static str, handling: DistinctHandling) -> AggregateUDF { + AggregateUDF::from(TaggedUdaf::new(name, handling)) + } + /// `min(DISTINCT b)` loses the flag but keeps its column name. #[test] fn eliminate_distinct_from_min() -> Result<()> { @@ -362,6 +430,149 @@ mod tests { ) } + /// A user defined `Ignored` aggregate loses the flag, keeps its output + /// name, and carries `FILTER` and `ORDER BY` over untouched. + #[test] + fn eliminate_distinct_from_ignored_udaf() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + tagged("first_seen", DistinctHandling::Ignored) + .call(vec![col("b")]) + .distinct() + .filter(col("c").gt(lit(0u32))) + .order_by(vec![col("b").sort(true, false)]) + .build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[first_seen(test.b) FILTER (WHERE test.c > UInt32(0)) ORDER BY [test.b ASC NULLS LAST] AS first_seen(DISTINCT test.b) FILTER (WHERE test.c > UInt32(0)) ORDER BY [test.b ASC NULLS LAST]]] + TableScan: test + ") + } + + /// A user defined aggregate that deduplicates for real keeps the flag. + #[test] + fn keep_distinct_on_honored_udaf() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + tagged("counts_uniques", DistinctHandling::Honored) + .call(vec![col("b")]) + .distinct() + .build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[counts_uniques(DISTINCT test.b)]] + TableScan: test + ") + } + + /// An aggregate that does not implement `DISTINCT` keeps the flag too: the + /// planner still has to deduplicate the input for it. + #[test] + fn keep_distinct_on_unsupported_udaf() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + tagged("needs_dedup", DistinctHandling::Unsupported) + .call(vec![col("b")]) + .distinct() + .build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[needs_dedup(DISTINCT test.b)]] + TableScan: test + ") + } + + /// The conservative gate covers user defined functions as well: an + /// `Ignored` one beside an `Honored` one keeps both flags. + #[test] + fn mixed_udaf_node_is_left_alone() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + tagged("first_seen", DistinctHandling::Ignored) + .call(vec![col("b")]) + .distinct() + .build()?, + tagged("counts_uniques", DistinctHandling::Honored) + .call(vec![col("c")]) + .distinct() + .build()?, + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[first_seen(DISTINCT test.b), counts_uniques(DISTINCT test.c)]] + TableScan: test + ") + } + + /// An aggregate that already carries an alias keeps that one name: the + /// rule restores the name it found, so no second alias is stacked on top. + #[test] + fn already_aliased_aggregate_gets_no_second_alias() -> Result<()> { + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![ + tagged("first_seen", DistinctHandling::Ignored) + .call(vec![col("b")]) + .distinct() + .build()? + .alias("m"), + ], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[first_seen(test.b) AS m]] + TableScan: test + ") + } + + /// `AggregateUDF::with_aliases` wraps the function in another + /// `AggregateUDFImpl`, which has to pass the tag through. + #[test] + fn with_aliases_delegates_distinct_handling() -> Result<()> { + let aliased = + tagged("first_seen", DistinctHandling::Ignored).with_aliases(["first_hit"]); + assert_eq!(aliased.distinct_handling(), DistinctHandling::Ignored); + + let table_scan = test_table_scan()?; + let plan = LogicalPlanBuilder::from(table_scan) + .aggregate( + vec![col("a")], + vec![aliased.call(vec![col("b")]).distinct().build()?], + )? + .build()?; + + assert_optimized_plan_equal!(plan, @r" + Aggregate: groupBy=[[test.a]], aggr=[[first_seen(test.b) AS first_seen(DISTINCT test.b)]] + TableScan: test + ") + } + /// A plan with no Aggregate takes the no-op path. #[test] fn non_aggregate_plan_is_unchanged() -> Result<()> { diff --git a/datafusion/sqllogictest/test_files/aggregates_simplify.slt b/datafusion/sqllogictest/test_files/aggregates_simplify.slt index a97135470caad..4169707913fb2 100644 --- a/datafusion/sqllogictest/test_files/aggregates_simplify.slt +++ b/datafusion/sqllogictest/test_files/aggregates_simplify.slt @@ -485,6 +485,27 @@ physical_plan 03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[bit_or(distinct_simplify_t.v) as bit_or(DISTINCT distinct_simplify_t.v)] 04)------DataSourceExec: partitions=1, partition_sizes=[1] +# approx_distinct: setting an HLL register a value already reached is a no-op, +# so the deduplicating inner group by that SingleDistinctToGroupBy used to plan +# here was pure cost. This is the largest plan change the rule makes. +query II +SELECT g, approx_distinct(DISTINCT v) FROM distinct_simplify_t GROUP BY g ORDER BY g; +---- +1 2 +2 1 + +query TT +EXPLAIN SELECT g, approx_distinct(DISTINCT v) FROM distinct_simplify_t GROUP BY g; +---- +logical_plan +01)Aggregate: groupBy=[[distinct_simplify_t.g]], aggr=[[approx_distinct(distinct_simplify_t.v) AS approx_distinct(DISTINCT distinct_simplify_t.v)]] +02)--TableScan: distinct_simplify_t projection=[g, v] +physical_plan +01)AggregateExec: mode=FinalPartitioned, gby=[g@0 as g], aggr=[approx_distinct(distinct_simplify_t.v) as approx_distinct(DISTINCT distinct_simplify_t.v)] +02)--RepartitionExec: partitioning=Hash([g@0], 4), input_partitions=1 +03)----AggregateExec: mode=Partial, gby=[g@0 as g], aggr=[approx_distinct(distinct_simplify_t.v) as approx_distinct(DISTINCT distinct_simplify_t.v)] +04)------DataSourceExec: partitions=1, partition_sizes=[1] + # Negative case: bit_xor cancels duplicate pairs, so DISTINCT is kept and # SingleDistinctToGroupBy still rewrites the plan. query II From 9e967792f4539804960a69ad50a906cf235e590a Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Wed, 16 Sep 2026 10:14:45 +0200 Subject: [PATCH 22/23] refactor: rename DistinctHandling::Ignored/Honored to Insensitive/Sensitive The old names described what the planner does with the flag, which reads backwards at the call site: a function returning `Ignored` is the one whose `DISTINCT` can be dropped, not one that ignores the modifier's meaning. The new names say what the function is: insensitive or sensitive to duplicates in its input. `Unsupported` is unchanged. Identifiers and prose that echoed the old names follow, including `strip_ignored_distinct` and four test names in the rule. Co-Authored-By: Claude Opus 5 --- datafusion/expr/src/udaf.rs | 10 ++-- .../functions-aggregate/src/any_value.rs | 4 +- .../src/approx_distinct.rs | 2 +- .../functions-aggregate/src/bit_and_or_xor.rs | 4 +- .../functions-aggregate/src/bool_and_or.rs | 4 +- .../functions-aggregate/src/first_last.rs | 8 +-- .../functions-aggregate/src/grouping.rs | 2 +- datafusion/functions-aggregate/src/min_max.rs | 4 +- .../functions-aggregate/src/variance.rs | 4 +- .../src/eliminate_aggregate_distinct.rs | 59 ++++++++++--------- .../functions/adding-udfs.md | 8 +-- 11 files changed, 55 insertions(+), 54 deletions(-) diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index d28e697d309c9..de65702ad9aa6 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -947,7 +947,7 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { /// How this function treats the `DISTINCT` modifier. /// - /// Return [`DistinctHandling::Ignored`] for duplicate-insensitive + /// Return [`DistinctHandling::Insensitive`] for duplicate-insensitive /// functions so that `f(DISTINCT x)` is planned as `f(x)`. /// /// Return [`DistinctHandling::Unsupported`] if the accumulator does not @@ -956,7 +956,7 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { /// the input or reject the query. Nothing reads this variant yet: /// rejecting such queries at planning time is a follow-up change. fn distinct_handling(&self) -> DistinctHandling { - DistinctHandling::Honored + DistinctHandling::Sensitive } /// Returns the documentation for this Aggregate UDF. @@ -1738,7 +1738,7 @@ pub enum SetMonotonicity { /// How an aggregate function treats the `DISTINCT` modifier. /// -/// Mathematically, `Ignored` means the function's merge operation is +/// Mathematically, `Insensitive` means the function's merge operation is /// idempotent (its state forms a semilattice): f(S ⊎ S) = f(S), so /// removing duplicates from the input cannot change the result. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -1746,11 +1746,11 @@ pub enum SetMonotonicity { 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, + Insensitive, /// The accumulator reads `AccumulatorArgs::is_distinct` and deduplicates /// its input, so the planner must leave the flag alone. `count`, `sum`, /// `avg`, `var_samp`, `array_agg`, ... This is the default. - Honored, + Sensitive, /// The accumulator does not implement `DISTINCT`: it does not read /// `is_distinct`, or it rejects `DISTINCT` with an error. The planner has /// to deduplicate the input first (today `SingleDistinctToGroupBy` does diff --git a/datafusion/functions-aggregate/src/any_value.rs b/datafusion/functions-aggregate/src/any_value.rs index a7bbdc7359c0e..b19e345d5c97b 100644 --- a/datafusion/functions-aggregate/src/any_value.rs +++ b/datafusion/functions-aggregate/src/any_value.rs @@ -123,8 +123,8 @@ impl AggregateUDFImpl for AnyValue { self.doc() } - // TODO: this is arguably `DistinctHandling::Ignored` — the accumulator + // TODO: this is arguably `DistinctHandling::Insensitive` — the accumulator // ignores `is_distinct` and returns an unspecified input value either // way. Grouped with `first_value`/`last_value` and left at the default - // `Honored` until that family is settled together. + // `Sensitive` until that family is settled together. } diff --git a/datafusion/functions-aggregate/src/approx_distinct.rs b/datafusion/functions-aggregate/src/approx_distinct.rs index 161cdb68ad72e..74f116cf48725 100644 --- a/datafusion/functions-aggregate/src/approx_distinct.rs +++ b/datafusion/functions-aggregate/src/approx_distinct.rs @@ -874,7 +874,7 @@ impl AggregateUDFImpl for ApproxDistinct { fn distinct_handling(&self) -> DistinctHandling { // Updating an HLL register with a value already seen is a no-op. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } diff --git a/datafusion/functions-aggregate/src/bit_and_or_xor.rs b/datafusion/functions-aggregate/src/bit_and_or_xor.rs index 8c6b6fe1b8e38..02d77c84ba0bb 100644 --- a/datafusion/functions-aggregate/src/bit_and_or_xor.rs +++ b/datafusion/functions-aggregate/src/bit_and_or_xor.rs @@ -325,10 +325,10 @@ impl AggregateUDFImpl for BitwiseOperation { // Bitwise AND/OR are idempotent: duplicates cannot change the // result. Only XOR has a distinct accumulator. BitwiseOperationType::And | BitwiseOperationType::Or => { - DistinctHandling::Ignored + DistinctHandling::Insensitive } // XOR cancels duplicate pairs, so `DISTINCT` is meaningful. - BitwiseOperationType::Xor => DistinctHandling::Honored, + BitwiseOperationType::Xor => DistinctHandling::Sensitive, } } } diff --git a/datafusion/functions-aggregate/src/bool_and_or.rs b/datafusion/functions-aggregate/src/bool_and_or.rs index 34449d63ef2cb..39baf5e0f48dd 100644 --- a/datafusion/functions-aggregate/src/bool_and_or.rs +++ b/datafusion/functions-aggregate/src/bool_and_or.rs @@ -187,7 +187,7 @@ impl AggregateUDFImpl for BoolAnd { fn distinct_handling(&self) -> DistinctHandling { // Boolean AND/OR are idempotent: duplicates cannot change the result. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } @@ -322,7 +322,7 @@ impl AggregateUDFImpl for BoolOr { fn distinct_handling(&self) -> DistinctHandling { // Boolean AND/OR are idempotent: duplicates cannot change the result. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } diff --git a/datafusion/functions-aggregate/src/first_last.rs b/datafusion/functions-aggregate/src/first_last.rs index c2992d276f80f..d11c12c0ec393 100644 --- a/datafusion/functions-aggregate/src/first_last.rs +++ b/datafusion/functions-aggregate/src/first_last.rs @@ -381,10 +381,10 @@ impl AggregateUDFImpl for FirstValue { self.doc() } - // TODO: whether this is `DistinctHandling::Ignored` depends on `ORDER BY`. + // TODO: whether this is `DistinctHandling::Insensitive` depends on `ORDER BY`. // `first_value(DISTINCT x ORDER BY y)` deduplicates `x` and leaves the `y` // ordering meaningless, while `first_value(DISTINCT x ORDER BY x)` is just - // `min(x)`. Left at the default `Honored` until that is settled, even + // `min(x)`. Left at the default `Sensitive` until that is settled, even // though the accumulator ignores `is_distinct` today. } @@ -1301,10 +1301,10 @@ impl AggregateUDFImpl for LastValue { create_groups_accumulator(&args, false, self.is_input_pre_ordered, self.name()) } - // TODO: whether this is `DistinctHandling::Ignored` depends on `ORDER BY`. + // TODO: whether this is `DistinctHandling::Insensitive` depends on `ORDER BY`. // `last_value(DISTINCT x ORDER BY y)` deduplicates `x` and leaves the `y` // ordering meaningless, while `last_value(DISTINCT x ORDER BY x)` is - // `max(x)` when `x` has no NULL. Left at the default `Honored` until that is settled, even + // `max(x)` when `x` has no NULL. Left at the default `Sensitive` until that is settled, even // though the accumulator ignores `is_distinct` today. } diff --git a/datafusion/functions-aggregate/src/grouping.rs b/datafusion/functions-aggregate/src/grouping.rs index 1c35b2cd04a28..f9ba80a95ef11 100644 --- a/datafusion/functions-aggregate/src/grouping.rs +++ b/datafusion/functions-aggregate/src/grouping.rs @@ -118,6 +118,6 @@ impl AggregateUDFImpl for Grouping { // `ResolveGroupingFunction` replaces the call before the optimizer // runs, so this tag is not reachable from SQL and the accumulator // above is never built. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } diff --git a/datafusion/functions-aggregate/src/min_max.rs b/datafusion/functions-aggregate/src/min_max.rs index 5e11981bb8db3..94814a802d971 100644 --- a/datafusion/functions-aggregate/src/min_max.rs +++ b/datafusion/functions-aggregate/src/min_max.rs @@ -403,7 +403,7 @@ impl AggregateUDFImpl for Max { fn distinct_handling(&self) -> DistinctHandling { // `MAX` is idempotent: duplicates cannot change the maximum. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } @@ -703,7 +703,7 @@ impl AggregateUDFImpl for Min { fn distinct_handling(&self) -> DistinctHandling { // `MIN` is idempotent: duplicates cannot change the minimum. - DistinctHandling::Ignored + DistinctHandling::Insensitive } } diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index 3c4003f099c15..154a677cb28f0 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -151,7 +151,7 @@ impl AggregateUDFImpl for VarianceSample { self.doc() } - // Left at the default `Honored`: `DistinctVarianceAccumulator` + // Left at the default `Sensitive`: `DistinctVarianceAccumulator` // deduplicates the input when `is_distinct` is set. } @@ -256,7 +256,7 @@ impl AggregateUDFImpl for VariancePopulation { self.doc() } - // Left at the default `Honored`: `DistinctVarianceAccumulator` + // Left at the default `Sensitive`: `DistinctVarianceAccumulator` // deduplicates the input when `is_distinct` is set. } diff --git a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs index a44af92871557..66bfea45f047e 100644 --- a/datafusion/optimizer/src/eliminate_aggregate_distinct.rs +++ b/datafusion/optimizer/src/eliminate_aggregate_distinct.rs @@ -16,7 +16,7 @@ // under the License. //! [`EliminateAggregateDistinct`] drops the `DISTINCT` modifier from aggregate -//! functions that report [`DistinctHandling::Ignored`] +//! functions that report [`DistinctHandling::Insensitive`] use crate::optimizer::ApplyOrder; use crate::{OptimizerConfig, OptimizerRule}; @@ -94,44 +94,45 @@ impl OptimizerRule for EliminateAggregateDistinct { let name_preserver = NamePreserver::new(&plan); plan.map_expressions(|expr| { let saved_name = name_preserver.save(&expr); - expr.transform_down(strip_ignored_distinct) + expr.transform_down(strip_insensitive_distinct) .map(|t| t.update_data(|e| saved_name.restore(e))) }) } } -/// Whether the node has at least one `DISTINCT` and every one is `Ignored`. +/// Whether the node has at least one `DISTINCT` and every one is +/// `Insensitive`. /// -/// If only some of them were stripped, an `Honored` `DISTINCT` could stay +/// If only some of them were stripped, a `Sensitive` `DISTINCT` could stay /// beside a stripped aggregate. A stripped aggregate carries an alias, and /// [`crate::single_distinct_to_groupby::SingleDistinctToGroupBy`] does not /// rewrite a node whose `aggr_expr` contains an alias. So `min(DISTINCT x), /// count(DISTINCT x)` would lose the rewrite that `count` needs. When every -/// `DISTINCT` is `Ignored`, none is left after stripping, so that rule has +/// `DISTINCT` is `Insensitive`, none is left after stripping, so that rule has /// nothing to rewrite. This is conservative: `min(DISTINCT x), /// count(DISTINCT y)` keeps the `min` flag. That flag costs nothing at run /// time, because `min` ignores `is_distinct` when it selects its accumulator. fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { let mut found_distinct = false; - let mut all_ignored = true; + let mut all_insensitive = true; for expr in aggr_expr { expr.apply(|e| { if let Expr::AggregateFunction(AggregateFunction { func, params }) = e && params.distinct { found_distinct = true; - if func.distinct_handling() != DistinctHandling::Ignored { - all_ignored = false; + if func.distinct_handling() != DistinctHandling::Insensitive { + all_insensitive = false; return Ok(TreeNodeRecursion::Stop); } } Ok(TreeNodeRecursion::Continue) })?; - if !all_ignored { + if !all_insensitive { break; } } - Ok(found_distinct && all_ignored) + Ok(found_distinct && all_insensitive) } /// Drops `DISTINCT` from `expr` if it is an aggregate that ignores duplicates. @@ -142,15 +143,15 @@ fn can_strip_every_distinct(aggr_expr: &[Expr]) -> Result { /// /// An idempotent merge is not always commutative: `first_value` is /// idempotent but order-sensitive. The rule does not need commutativity. -/// `Ignored` means that the result does not change when duplicates are +/// `Insensitive` means that the result does not change when duplicates are /// removed, and stripping `DISTINCT` only stops that removal. `order_by` and /// `filter` are carried over untouched, so the function sees the same rows in /// the same order, plus the duplicates that it ignores. -fn strip_ignored_distinct(expr: Expr) -> Result> { +fn strip_insensitive_distinct(expr: Expr) -> Result> { Ok(match expr { Expr::AggregateFunction(mut agg) if agg.params.distinct - && agg.func.distinct_handling() == DistinctHandling::Ignored => + && agg.func.distinct_handling() == DistinctHandling::Insensitive => { agg.params.distinct = false; Transformed::yes(Expr::AggregateFunction(agg)) @@ -322,7 +323,7 @@ mod tests { /// Several duplicate-insensitive aggregates all lose the flag together. #[test] - fn eliminate_distinct_from_every_ignored_aggregate() -> Result<()> { + fn eliminate_distinct_from_every_insensitive_aggregate() -> Result<()> { let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) .aggregate( @@ -357,10 +358,10 @@ mod tests { ") } - /// The gate only inspects `aggr_expr`, so a distinct aggregate that honors - /// the flag in the group expressions must keep it. + /// The gate only inspects `aggr_expr`, so a `Sensitive` distinct aggregate + /// in the group expressions must keep its flag. #[test] - fn keep_honored_distinct_in_group_expr() -> Result<()> { + fn keep_sensitive_distinct_in_group_expr() -> Result<()> { let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) .aggregate( @@ -430,16 +431,16 @@ mod tests { ) } - /// A user defined `Ignored` aggregate loses the flag, keeps its output + /// A user defined `Insensitive` aggregate loses the flag, keeps its output /// name, and carries `FILTER` and `ORDER BY` over untouched. #[test] - fn eliminate_distinct_from_ignored_udaf() -> Result<()> { + fn eliminate_distinct_from_insensitive_udaf() -> Result<()> { let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) .aggregate( vec![col("a")], vec![ - tagged("first_seen", DistinctHandling::Ignored) + tagged("first_seen", DistinctHandling::Insensitive) .call(vec![col("b")]) .distinct() .filter(col("c").gt(lit(0u32))) @@ -457,13 +458,13 @@ mod tests { /// A user defined aggregate that deduplicates for real keeps the flag. #[test] - fn keep_distinct_on_honored_udaf() -> Result<()> { + fn keep_distinct_on_sensitive_udaf() -> Result<()> { let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) .aggregate( vec![col("a")], vec![ - tagged("counts_uniques", DistinctHandling::Honored) + tagged("counts_uniques", DistinctHandling::Sensitive) .call(vec![col("b")]) .distinct() .build()?, @@ -501,7 +502,7 @@ mod tests { } /// The conservative gate covers user defined functions as well: an - /// `Ignored` one beside an `Honored` one keeps both flags. + /// `Insensitive` one beside a `Sensitive` one keeps both flags. #[test] fn mixed_udaf_node_is_left_alone() -> Result<()> { let table_scan = test_table_scan()?; @@ -509,11 +510,11 @@ mod tests { .aggregate( vec![col("a")], vec![ - tagged("first_seen", DistinctHandling::Ignored) + tagged("first_seen", DistinctHandling::Insensitive) .call(vec![col("b")]) .distinct() .build()?, - tagged("counts_uniques", DistinctHandling::Honored) + tagged("counts_uniques", DistinctHandling::Sensitive) .call(vec![col("c")]) .distinct() .build()?, @@ -536,7 +537,7 @@ mod tests { .aggregate( vec![col("a")], vec![ - tagged("first_seen", DistinctHandling::Ignored) + tagged("first_seen", DistinctHandling::Insensitive) .call(vec![col("b")]) .distinct() .build()? @@ -555,9 +556,9 @@ mod tests { /// `AggregateUDFImpl`, which has to pass the tag through. #[test] fn with_aliases_delegates_distinct_handling() -> Result<()> { - let aliased = - tagged("first_seen", DistinctHandling::Ignored).with_aliases(["first_hit"]); - assert_eq!(aliased.distinct_handling(), DistinctHandling::Ignored); + let aliased = tagged("first_seen", DistinctHandling::Insensitive) + .with_aliases(["first_hit"]); + assert_eq!(aliased.distinct_handling(), DistinctHandling::Insensitive); let table_scan = test_table_scan()?; let plan = LogicalPlanBuilder::from(table_scan) diff --git a/docs/source/library-user-guide/functions/adding-udfs.md b/docs/source/library-user-guide/functions/adding-udfs.md index c330d3db9612d..78e90dfa6b8d4 100644 --- a/docs/source/library-user-guide/functions/adding-udfs.md +++ b/docs/source/library-user-guide/functions/adding-udfs.md @@ -1118,21 +1118,21 @@ impl Accumulator for GeometricMean { ### Declaring how an Aggregate UDF treats `DISTINCT` -By default DataFusion assumes an aggregate honors the `DISTINCT` modifier, which means the accumulator is expected to +By default DataFusion assumes an aggregate is sensitive to the `DISTINCT` modifier, which means the accumulator is expected to read `AccumulatorArgs::is_distinct` and deduplicate its input. Override [`AggregateUDFImpl::distinct_handling`] when that is not what your function does: -- Return `DistinctHandling::Ignored` when duplicates cannot change the result, that is, when merging a value the +- Return `DistinctHandling::Insensitive` when duplicates cannot change the result, that is, when merging a value the accumulator has already seen is a no-op. `min`, `max`, `bool_and` and `bit_or` are all in this group. The optimizer then plans `f(DISTINCT x)` as `f(x)`, which skips both the per-group hash set and the extra grouping stage that `SingleDistinctToGroupBy` would otherwise introduce. - Return `DistinctHandling::Unsupported` when the accumulator does not implement `DISTINCT`: it does not read `is_distinct`, or it rejects `DISTINCT` with an error. The planner must then deduplicate the input first or reject the query. Today this is a declaration only; rejecting such queries at planning time is a follow-up change. -- Leave the default `DistinctHandling::Honored` when the accumulator reads `AccumulatorArgs::is_distinct` and +- Leave the default `DistinctHandling::Sensitive` when the accumulator reads `AccumulatorArgs::is_distinct` and deduplicates its input itself. -Getting this wrong changes query results, so only claim `Ignored` if your merge is genuinely idempotent. +Getting this wrong changes query results, so only claim `Insensitive` if your merge is genuinely idempotent. ### Registering an Aggregate UDF From 9089a07b081e8dbccfb07425e01facf977602e35 Mon Sep 17 00:00:00 2001 From: Michael Kleen Date: Wed, 16 Sep 2026 11:49:47 +0200 Subject: [PATCH 23/23] Fix formatting --- datafusion/core/src/optimizer_rule_reference.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/core/src/optimizer_rule_reference.md b/datafusion/core/src/optimizer_rule_reference.md index 249f2dea08e58..298a01edd64dc 100644 --- a/datafusion/core/src/optimizer_rule_reference.md +++ b/datafusion/core/src/optimizer_rule_reference.md @@ -36,7 +36,7 @@ Rule order matters. The default pipeline may change between releases. ### Logical Optimizer Rules | order | rule | summary | -|-------| ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | +| ----- | ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | | 1 | `rewrite_set_comparison` | Rewrites `ANY` and `ALL` set-comparison subqueries into `EXISTS`-based boolean expressions with correct SQL NULL semantics. | | 2 | `optimize_unions` | Flattens nested unions and removes unions with a single input. | | 3 | `unions_to_filter` | Merges `UNION DISTINCT` branches that share the same source into a single filtered branch with a disjunctive predicate. |