From a9b901ac0cc361cf8287a34d7c2e784a0aaddefd Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 21 Nov 2024 11:38:54 +0900
Subject: [PATCH 01/39] Add within group variable to aggregate function and
 arguments

---
 datafusion/expr/src/expr.rs         |  4 ++++
 datafusion/sql/src/expr/function.rs | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index 3e692189e4885..27c1206ef08c3 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -665,6 +665,8 @@ pub struct AggregateFunction {
     /// Optional ordering
     pub order_by: Option<Vec<Sort>>,
     pub null_treatment: Option<NullTreatment>,
+    /// Optional within group for ordered-set aggregate functions
+    pub within_group: Option<Vec<Sort>>,
 }
 
 impl AggregateFunction {
@@ -676,6 +678,7 @@ impl AggregateFunction {
         filter: Option<Box<Expr>>,
         order_by: Option<Vec<Sort>>,
         null_treatment: Option<NullTreatment>,
+        within_group: Option<Vec<Sort>>,
     ) -> Self {
         Self {
             func,
@@ -684,6 +687,7 @@ impl AggregateFunction {
             filter,
             order_by,
             null_treatment,
+            within_group,
         }
     }
 }
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 619eadcf0fb86..7868e16aad243 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -97,6 +97,8 @@ struct FunctionArgs {
     null_treatment: Option<NullTreatment>,
     /// DISTINCT
     distinct: bool,
+    /// WITHIN_GROUP clause, if any
+    within_group: Vec<OrderByExpr>,
 }
 
 impl FunctionArgs {
@@ -121,6 +123,7 @@ impl FunctionArgs {
                 filter,
                 null_treatment,
                 distinct: false,
+                within_group,
             });
         };
 
@@ -191,6 +194,7 @@ impl FunctionArgs {
             filter,
             null_treatment,
             distinct,
+            within_group,
         })
     }
 }
@@ -211,6 +215,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             filter,
             null_treatment,
             distinct,
+            within_group,
         } = function_args;
 
         // If function is a window function (it has an OVER clause),
@@ -337,6 +342,17 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                 )?;
                 let order_by = (!order_by.is_empty()).then_some(order_by);
                 let args = self.function_args_to_expr(args, schema, planner_context)?;
+
+                let within_group = self.order_by_to_sort_expr( // TODO : need to double check if it can be used as it is
+                    within_group,
+                    schema,
+                    planner_context,
+                    false,
+                    None,
+                )?;
+
+                let within_group = (!within_group.is_empty()).then_some(within_group);
+
                 let filter: Option<Box<Expr>> = filter
                     .map(|e| self.sql_expr_to_logical_expr(*e, schema, planner_context))
                     .transpose()?
@@ -348,6 +364,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                     filter,
                     order_by,
                     null_treatment,
+                    within_group,
                 )));
             }
         }

From 070a96b31c2371f6d75b8d31661d6cd1a6a55e76 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Tue, 21 Jan 2025 16:49:49 +0900
Subject: [PATCH 02/39] Support within group and disable null handling for
 ordered set aggregate functions (#13511)

---
 .../core/tests/execution/logical_plan.rs      |  1 +
 datafusion/expr/src/expr.rs                   | 14 ++++++--
 datafusion/expr/src/expr_fn.rs                | 24 +++++++++++++
 datafusion/expr/src/udaf.rs                   | 13 +++++++
 .../src/approx_percentile_cont.rs             |  4 +++
 .../src/approx_percentile_cont_with_weight.rs |  4 +++
 datafusion/functions-aggregate/src/macros.rs  |  2 ++
 datafusion/sql/src/expr/function.rs           | 34 ++++++++++++++++---
 datafusion/sql/src/unparser/expr.rs           | 10 +++++-
 9 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/datafusion/core/tests/execution/logical_plan.rs b/datafusion/core/tests/execution/logical_plan.rs
index 168bf484e5411..f0420c641d988 100644
--- a/datafusion/core/tests/execution/logical_plan.rs
+++ b/datafusion/core/tests/execution/logical_plan.rs
@@ -64,6 +64,7 @@ async fn count_only_nulls() -> Result<()> {
             filter: None,
             order_by: None,
             null_treatment: None,
+            within_group: None,
         })],
     )?);
 
diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index 1f4973ff6275f..2b49c9325fd5d 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -290,7 +290,7 @@ pub enum Expr {
     /// Represents the call of a scalar function with a set of arguments.
     ScalarFunction(ScalarFunction),
     /// Calls an aggregate function with arguments, and optional
-    /// `ORDER BY`, `FILTER`, `DISTINCT` and `NULL TREATMENT`.
+    /// `ORDER BY`, `FILTER`, `DISTINCT`, `NULL TREATMENT`, and `WITHIN GROUP`.
     ///
     /// See also [`ExprFunctionExt`] to set these fields.
     ///
@@ -705,7 +705,7 @@ pub struct AggregateFunction {
     /// Optional ordering
     pub order_by: Option<Vec<Sort>>,
     pub null_treatment: Option<NullTreatment>,
-    /// Optional within group for ordered-set aggregate functions
+    /// Optional WITHIN GROUP for ordered-set aggregate functions
     pub within_group: Option<Vec<Sort>>,
 }
 
@@ -873,6 +873,8 @@ pub struct AggregateUDF {
     pub filter: Option<Box<Expr>>,
     /// Optional ORDER BY applied prior to aggregating
     pub order_by: Option<Vec<Expr>>,
+    /// Optional WITHIN GROUP for ordered-set aggregate functions
+    pub within_group: Option<Vec<Expr>>,
 }
 
 impl AggregateUDF {
@@ -882,12 +884,14 @@ impl AggregateUDF {
         args: Vec<Expr>,
         filter: Option<Box<Expr>>,
         order_by: Option<Vec<Expr>>,
+        within_group: Option<Vec<Expr>>,
     ) -> Self {
         Self {
             fun,
             args,
             filter,
             order_by,
+            within_group,
         }
     }
 }
@@ -1762,6 +1766,7 @@ impl HashNode for Expr {
                 filter: _filter,
                 order_by: _order_by,
                 null_treatment,
+                within_group: _within_group,
             }) => {
                 func.hash(state);
                 distinct.hash(state);
@@ -1874,6 +1879,7 @@ impl<'a> Display for SchemaDisplay<'a> {
                 filter,
                 order_by,
                 null_treatment,
+                within_group,
             }) => {
                 write!(
                     f,
@@ -1895,6 +1901,10 @@ impl<'a> Display for SchemaDisplay<'a> {
                     write!(f, " ORDER BY [{}]", schema_name_from_sorts(order_by)?)?;
                 };
 
+                if let Some(within_group) = within_group {
+                    write!(f, " WITHIN GROUP [{}]", schema_name_from_sorts(within_group)?)?;
+                };
+
                 Ok(())
             }
             // Expr is not shown since it is aliased
diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs
index 7fd4e64e0e627..a25fb2bbeb2d7 100644
--- a/datafusion/expr/src/expr_fn.rs
+++ b/datafusion/expr/src/expr_fn.rs
@@ -747,6 +747,8 @@ pub trait ExprFunctionExt {
     fn partition_by(self, partition_by: Vec<Expr>) -> ExprFuncBuilder;
     /// Add appropriate window frame conditions
     fn window_frame(self, window_frame: WindowFrame) -> ExprFuncBuilder;
+    /// Add `WITHIN GROUP <within_group>`
+    fn within_group(self, within_group: Vec<Sort>) -> ExprFuncBuilder;
 }
 
 #[derive(Debug, Clone)]
@@ -767,6 +769,7 @@ pub struct ExprFuncBuilder {
     null_treatment: Option<NullTreatment>,
     partition_by: Option<Vec<Expr>>,
     window_frame: Option<WindowFrame>,
+    within_group: Option<Vec<Sort>>,
 }
 
 impl ExprFuncBuilder {
@@ -780,6 +783,7 @@ impl ExprFuncBuilder {
             null_treatment: None,
             partition_by: None,
             window_frame: None,
+            within_group: None,
         }
     }
 
@@ -798,6 +802,7 @@ impl ExprFuncBuilder {
             null_treatment,
             partition_by,
             window_frame,
+            within_group,
         } = self;
 
         let Some(fun) = fun else {
@@ -812,6 +817,7 @@ impl ExprFuncBuilder {
                 udaf.filter = filter.map(Box::new);
                 udaf.distinct = distinct;
                 udaf.null_treatment = null_treatment;
+                udaf.within_group = within_group;
                 Expr::AggregateFunction(udaf)
             }
             ExprFuncKind::Window(mut udwf) => {
@@ -866,6 +872,11 @@ impl ExprFunctionExt for ExprFuncBuilder {
         self.window_frame = Some(window_frame);
         self
     }
+
+    fn within_group(mut self, within_group: Vec<Sort>) -> ExprFuncBuilder {
+        self.within_group = Some(within_group);
+        self
+    }
 }
 
 impl ExprFunctionExt for Expr {
@@ -946,6 +957,19 @@ impl ExprFunctionExt for Expr {
             _ => ExprFuncBuilder::new(None),
         }
     }
+
+    fn within_group(self, within_group: Vec<Sort>) -> ExprFuncBuilder {
+        let mut builder = match self {
+            Expr::AggregateFunction(udaf) => {
+                ExprFuncBuilder::new(Some(ExprFuncKind::Aggregate(udaf)))
+            }
+            _ => ExprFuncBuilder::new(None),
+        };
+        if builder.fun.is_some() {
+            builder.within_group = Some(within_group);
+        }
+        builder
+    }
 }
 
 #[cfg(test)]
diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index dbbf88447ba39..729c3be426f88 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -152,6 +152,7 @@ impl AggregateUDF {
             None,
             None,
             None,
+            None,
         ))
     }
 
@@ -276,6 +277,11 @@ impl AggregateUDF {
         self.inner.default_value(data_type)
     }
 
+    /// See [`AggregateUDFImpl::supports_null_handling_clause`] for more details.
+    pub fn supports_null_handling_clause(&self) -> Option<bool> {
+        self.inner.supports_null_handling_clause()
+    }
+
     /// Returns the documentation for this Aggregate UDF.
     ///
     /// Documentation can be accessed programmatically as well as
@@ -632,6 +638,13 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
         ScalarValue::try_from(data_type)
     }
 
+    /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
+    /// If the function does not, return false
+    /// Otherwise return None (the default)
+    fn supports_null_handling_clause(&self) -> Option<bool> {
+        None
+    }
+
     /// Returns the documentation for this Aggregate UDF.
     ///
     /// Documentation can be accessed programmatically as well as
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 53fcfd641ddfc..b3c3f3d0c5614 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -271,6 +271,10 @@ impl AggregateUDFImpl for ApproxPercentileCont {
         Ok(arg_types[0].clone())
     }
 
+    fn supports_null_handling_clause(&self) -> Option<bool> {
+        Some(false)
+    }
+
     fn documentation(&self) -> Option<&Documentation> {
         Some(get_approx_percentile_cont_doc())
     }
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 5458d0f792b92..4a82b6878084e 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
@@ -156,6 +156,10 @@ impl AggregateUDFImpl for ApproxPercentileContWithWeight {
         self.approx_percentile_cont.state_fields(args)
     }
 
+    fn supports_null_handling_clause(&self) -> Option<bool> {
+        Some(false)
+    }
+
     fn documentation(&self) -> Option<&Documentation> {
         Some(get_approx_percentile_cont_with_weight_doc())
     }
diff --git a/datafusion/functions-aggregate/src/macros.rs b/datafusion/functions-aggregate/src/macros.rs
index ffb5183278e67..f3d565d1894ce 100644
--- a/datafusion/functions-aggregate/src/macros.rs
+++ b/datafusion/functions-aggregate/src/macros.rs
@@ -29,6 +29,7 @@ macro_rules! make_udaf_expr {
                 None,
                 None,
                 None,
+                None,
             ))
         }
     };
@@ -52,6 +53,7 @@ macro_rules! make_udaf_expr_and_func {
                 None,
                 None,
                 None,
+                None,
             ))
         }
 
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 01f2897be651e..f79014dd7ae39 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -75,7 +75,7 @@ fn find_closest_match(candidates: Vec<String>, target: &str) -> Option<String> {
     })
 }
 
-/// Arguments to for a function call extracted from the SQL AST
+/// Arguments for a function call extracted from the SQL AST
 #[derive(Debug)]
 struct FunctionArgs {
     /// Function name
@@ -175,8 +175,12 @@ impl FunctionArgs {
             }
         }
 
-        if !within_group.is_empty() {
-            return not_impl_err!("WITHIN GROUP is not supported yet: {within_group:?}");
+        if !within_group.is_empty() && order_by.is_some() {
+            return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used.");
+        }
+
+        if within_group.len() > 1 {
+            return not_impl_err!("Multiple column ordering in WITHIN GROUP clause is not supported: {within_group:?}");
         }
 
         let order_by = order_by.unwrap_or_default();
@@ -328,6 +332,17 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
+                if let Some(fm) = self.context_provider.get_ordered_set_aggregate_meta(&name) {
+                    if within_group.is_empty() {
+                        return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({}).", fm.name());
+                    }
+                }
+
+                if null_treatment.is_some() &&
+                    !fm.supports_null_handling_clause().unwrap_or(true) {
+                    return plan_err!("[IGNORE | RESPECT] NULLS are not permitted for {}.", fm.name());
+                }
+
                 let order_by = self.order_by_to_sort_expr(
                     order_by,
                     schema,
@@ -336,9 +351,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                     None,
                 )?;
                 let order_by = (!order_by.is_empty()).then_some(order_by);
-                let args = self.function_args_to_expr(args, schema, planner_context)?;
+                let mut args = self.function_args_to_expr(args, schema, planner_context)?;
 
-                let within_group = self.order_by_to_sort_expr( // TODO : need to double check if it can be used as it is
+                let within_group = self.order_by_to_sort_expr(
                     within_group,
                     schema,
                     planner_context,
@@ -346,6 +361,15 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                     None,
                 )?;
 
+                // add target column expression in within group clause to function arguments
+                if !within_group.is_empty() {
+                    args = within_group
+                        .iter()
+                        .map(|sort| sort.expr.clone())
+                        .chain(args)
+                        .collect::<Vec<_>>();
+                }
+
                 let within_group = (!within_group.is_empty()).then_some(within_group);
 
                 let filter: Option<Box<Expr>> = filter
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index f1f28258f9bd6..1db6f0434aa15 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -274,6 +274,14 @@ impl Unparser<'_> {
                     Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
                     None => None,
                 };
+
+                let within_group = agg.within_group
+                    .as_ref()
+                    .unwrap_or(&Vec::new())
+                    .iter()
+                    .map(|sort_expr| self.sort_to_sql(sort_expr))
+                    .collect::<Result<Vec<_>>>()?;
+
                 Ok(ast::Expr::Function(Function {
                     name: ObjectName(vec![Ident {
                         value: func_name.to_string(),
@@ -289,7 +297,7 @@ impl Unparser<'_> {
                     filter,
                     null_treatment: None,
                     over: None,
-                    within_group: vec![],
+                    within_group,
                     parameters: ast::FunctionArguments::None,
                 }))
             }

From 3fd92fd8ea52a94b8904548ead8257602dd70fdb Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Tue, 21 Jan 2025 16:57:13 +0900
Subject: [PATCH 03/39] Refactored function to match updated signature

---
 .../tests/dataframe/dataframe_functions.rs    | 36 +++++++++----------
 .../src/approx_percentile_cont.rs             | 23 +++++++++---
 .../tests/cases/roundtrip_logical_plan.rs     |  4 +--
 3 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 1bd90fce839d0..fa37a03dfae4d 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -360,14 +360,14 @@ async fn test_fn_approx_median() -> Result<()> {
 
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
-    let expr = approx_percentile_cont(col("b"), lit(0.5), None);
+    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
 
     let expected = [
-        "+---------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5)) |",
-        "+---------------------------------------------+",
-        "| 10                                          |",
-        "+---------------------------------------------+",
+        "+-----------------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [b ASC NULLS LAST] |",
+        "+-----------------------------------------------------------------------------+",
+        "| 10                                                                          |",
+        "+-----------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
@@ -381,27 +381,27 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         None::<&str>,
         "arg_2".to_string(),
     ));
-    let expr = approx_percentile_cont(col("b"), alias_expr, None);
+    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
-        "+--------------------------------------+",
-        "| approx_percentile_cont(test.b,arg_2) |",
-        "+--------------------------------------+",
-        "| 10                                   |",
-        "+--------------------------------------+",
+        "+----------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,arg_2) WITHIN GROUP [b ASC NULLS LAST] |",
+        "+----------------------------------------------------------------------+",
+        "| 10                                                                   |",
+        "+----------------------------------------------------------------------+",
     ];
     let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
 
     assert_batches_eq!(expected, &batches);
 
     // with number of centroids set
-    let expr = approx_percentile_cont(col("b"), lit(0.5), Some(lit(2)));
+    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), Some(lit(2)));
     let expected = [
-        "+------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) |",
-        "+------------------------------------------------------+",
-        "| 30                                                   |",
-        "+------------------------------------------------------+",
+        "+--------------------------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [b ASC NULLS LAST] |",
+        "+--------------------------------------------------------------------------------------+",
+        "| 30                                                                                   |",
+        "+--------------------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index b3c3f3d0c5614..3068508c350f1 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -36,6 +36,7 @@ use datafusion_common::{
     DataFusionError, Result, ScalarValue,
 };
 use datafusion_expr::aggregate_doc_sections::DOC_SECTION_APPROXIMATE;
+use datafusion_expr::expr::{AggregateFunction, Sort};
 use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
 use datafusion_expr::type_coercion::aggregates::{INTEGERS, NUMERICS};
 use datafusion_expr::utils::format_state_name;
@@ -52,16 +53,30 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
 
 /// Computes the approximate percentile continuous of a set of numbers
 pub fn approx_percentile_cont(
-    expression: Expr,
+    within_group: Vec<Sort>,
     percentile: Expr,
     centroids: Option<Expr>,
 ) -> Expr {
+    let expr = within_group
+        .first()
+        .map(|sort| sort.expr.clone())
+        .unwrap_or_default();
+
     let args = if let Some(centroids) = centroids {
-        vec![expression, percentile, centroids]
+        vec![expr, percentile, centroids]
     } else {
-        vec![expression, percentile]
+        vec![expr, percentile]
     };
-    approx_percentile_cont_udaf().call(args)
+
+    Expr::AggregateFunction(AggregateFunction::new_udf(
+        approx_percentile_cont_udaf(),
+        args,
+        false,
+        None,
+        None,
+        None,
+        Some(within_group),
+    ))
 }
 
 pub struct ApproxPercentileCont {
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index cfb6862a0ca3e..685dc834173df 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -930,8 +930,8 @@ async fn roundtrip_expr_api() -> Result<()> {
         stddev_pop(lit(2.2)),
         approx_distinct(lit(2)),
         approx_median(lit(2)),
-        approx_percentile_cont(lit(2), lit(0.5), None),
-        approx_percentile_cont(lit(2), lit(0.5), Some(lit(50))),
+        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), None),
+        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), Some(lit(50))),
         approx_percentile_cont_with_weight(lit(2), lit(1), lit(0.5)),
         grouping(lit(1)),
         bit_and(lit(2)),

From 4082a78094e148bdf01bce4913d0dee10f23594b Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Tue, 21 Jan 2025 17:11:07 +0900
Subject: [PATCH 04/39] Modify proto to support within group clause

---
 datafusion/proto/proto/datafusion.proto       |  1 +
 datafusion/proto/src/generated/pbjson.rs      | 18 +++++++++
 datafusion/proto/src/generated/prost.rs       |  2 +
 .../proto/src/logical_plan/from_proto.rs      |  4 ++
 datafusion/proto/src/logical_plan/to_proto.rs |  5 +++
 .../substrait/src/logical_plan/consumer.rs    | 39 ++++++++++++++-----
 .../substrait/src/logical_plan/producer.rs    | 21 +++++++---
 7 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/datafusion/proto/proto/datafusion.proto b/datafusion/proto/proto/datafusion.proto
index 504e5e1ceead7..ce9d36d849a0e 100644
--- a/datafusion/proto/proto/datafusion.proto
+++ b/datafusion/proto/proto/datafusion.proto
@@ -499,6 +499,7 @@ message AggregateUDFExprNode {
   LogicalExprNode filter = 3;
   repeated SortExprNode order_by = 4;
   optional bytes fun_definition = 6;
+  repeated SortExprNode within_group = 7;
 }
 
 message ScalarUDFExprNode {
diff --git a/datafusion/proto/src/generated/pbjson.rs b/datafusion/proto/src/generated/pbjson.rs
index 29920814a8025..fc76fbeb66bdb 100644
--- a/datafusion/proto/src/generated/pbjson.rs
+++ b/datafusion/proto/src/generated/pbjson.rs
@@ -596,6 +596,9 @@ impl serde::Serialize for AggregateUdfExprNode {
         if self.fun_definition.is_some() {
             len += 1;
         }
+        if !self.within_group.is_empty() {
+            len += 1;
+        }
         let mut struct_ser = serializer.serialize_struct("datafusion.AggregateUDFExprNode", len)?;
         if !self.fun_name.is_empty() {
             struct_ser.serialize_field("funName", &self.fun_name)?;
@@ -617,6 +620,9 @@ impl serde::Serialize for AggregateUdfExprNode {
             #[allow(clippy::needless_borrows_for_generic_args)]
             struct_ser.serialize_field("funDefinition", pbjson::private::base64::encode(&v).as_str())?;
         }
+        if !self.within_group.is_empty() {
+            struct_ser.serialize_field("withinGroup", &self.within_group)?;
+        }
         struct_ser.end()
     }
 }
@@ -636,6 +642,8 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
             "orderBy",
             "fun_definition",
             "funDefinition",
+            "within_group",
+            "withinGroup",
         ];
 
         #[allow(clippy::enum_variant_names)]
@@ -646,6 +654,7 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
             Filter,
             OrderBy,
             FunDefinition,
+            WithinGroup,
         }
         impl<'de> serde::Deserialize<'de> for GeneratedField {
             fn deserialize<D>(deserializer: D) -> std::result::Result<GeneratedField, D::Error>
@@ -673,6 +682,7 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                             "filter" => Ok(GeneratedField::Filter),
                             "orderBy" | "order_by" => Ok(GeneratedField::OrderBy),
                             "funDefinition" | "fun_definition" => Ok(GeneratedField::FunDefinition),
+                            "withinGroup" | "within_group" => Ok(GeneratedField::WithinGroup),
                             _ => Err(serde::de::Error::unknown_field(value, FIELDS)),
                         }
                     }
@@ -698,6 +708,7 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                 let mut filter__ = None;
                 let mut order_by__ = None;
                 let mut fun_definition__ = None;
+                let mut within_group__ = None;
                 while let Some(k) = map_.next_key()? {
                     match k {
                         GeneratedField::FunName => {
@@ -738,6 +749,12 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                                 map_.next_value::<::std::option::Option<::pbjson::private::BytesDeserialize<_>>>()?.map(|x| x.0)
                             ;
                         }
+                        GeneratedField::WithinGroup => {
+                            if within_group__.is_some() {
+                                return Err(serde::de::Error::duplicate_field("withinGroup"));
+                            }
+                            within_group__ = Some(map_.next_value()?);
+                        }
                     }
                 }
                 Ok(AggregateUdfExprNode {
@@ -747,6 +764,7 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                     filter: filter__,
                     order_by: order_by__.unwrap_or_default(),
                     fun_definition: fun_definition__,
+                    within_group: within_group__.unwrap_or_default(),
                 })
             }
         }
diff --git a/datafusion/proto/src/generated/prost.rs b/datafusion/proto/src/generated/prost.rs
index 07090b7cba110..e806a54489df8 100644
--- a/datafusion/proto/src/generated/prost.rs
+++ b/datafusion/proto/src/generated/prost.rs
@@ -719,6 +719,8 @@ pub struct AggregateUdfExprNode {
     pub order_by: ::prost::alloc::vec::Vec<SortExprNode>,
     #[prost(bytes = "vec", optional, tag = "6")]
     pub fun_definition: ::core::option::Option<::prost::alloc::vec::Vec<u8>>,
+    #[prost(message, repeated, tag = "7")]
+    pub within_group: ::prost::alloc::vec::Vec<SortExprNode>,
 }
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct ScalarUdfExprNode {
diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs
index 301efc42a7c47..7017501d54375 100644
--- a/datafusion/proto/src/logical_plan/from_proto.rs
+++ b/datafusion/proto/src/logical_plan/from_proto.rs
@@ -546,6 +546,10 @@ pub fn parse_expr(
                     _ => Some(parse_sorts(&pb.order_by, registry, codec)?),
                 },
                 None,
+                match pb.within_group.len() {
+                    0 => None,
+                    _ => Some(parse_sorts(&pb.within_group, registry, codec)?),
+                },
             )))
         }
 
diff --git a/datafusion/proto/src/logical_plan/to_proto.rs b/datafusion/proto/src/logical_plan/to_proto.rs
index caceb3db164c2..62f2f6eb385d7 100644
--- a/datafusion/proto/src/logical_plan/to_proto.rs
+++ b/datafusion/proto/src/logical_plan/to_proto.rs
@@ -351,6 +351,7 @@ pub fn serialize_expr(
             ref filter,
             ref order_by,
             null_treatment: _,
+            ref within_group,
         }) => {
             let mut buf = Vec::new();
             let _ = codec.try_encode_udaf(func, &mut buf);
@@ -369,6 +370,10 @@ pub fn serialize_expr(
                             None => vec![],
                         },
                         fun_definition: (!buf.is_empty()).then_some(buf),
+                        within_group: match within_group {
+                            Some(e) => serialize_sorts(e, codec)?,
+                            None => vec![],
+                        },
                     },
                 ))),
             }
diff --git a/datafusion/substrait/src/logical_plan/consumer.rs b/datafusion/substrait/src/logical_plan/consumer.rs
index 77e9eb81f546f..82484f4e51a8d 100644
--- a/datafusion/substrait/src/logical_plan/consumer.rs
+++ b/datafusion/substrait/src/logical_plan/consumer.rs
@@ -739,7 +739,17 @@ pub async fn from_substrait_rel(
                                 }
                                 _ => false,
                             };
-                            let order_by = if !f.sorts.is_empty() {
+
+                            let Some(function_name) = extensions.functions.get(&f.function_reference) else {
+                                return plan_err!(
+                                    "Aggregate function not registered: function anchor = {:?}",
+                                    f.function_reference
+                                );
+                            };
+
+                            let function_name = substrait_fun_name(function_name);
+
+                            let sorts = if !f.sorts.is_empty() {
                                 Some(
                                     from_substrait_sorts(
                                         state,
@@ -753,13 +763,20 @@ pub async fn from_substrait_rel(
                                 None
                             };
 
+                            let (within_group, order_by) = match state.ordered_set_udaf(function_name) {
+                                Ok(_) => (sorts, None),
+                                Err(_) => (None, sorts),
+                            };
+
                             from_substrait_agg_func(
                                 state,
+                                function_name,
                                 f,
                                 input.schema(),
                                 extensions,
                                 filter,
                                 order_by,
+                                within_group,
                                 distinct,
                             )
                             .await
@@ -1502,24 +1519,18 @@ pub async fn from_substrait_func_args(
 /// Convert Substrait AggregateFunction to DataFusion Expr
 pub async fn from_substrait_agg_func(
     state: &dyn SubstraitPlanningState,
+    function_name: &str,
     f: &AggregateFunction,
     input_schema: &DFSchema,
     extensions: &Extensions,
     filter: Option<Box<Expr>>,
     order_by: Option<Vec<SortExpr>>,
+    within_group: Option<Vec<SortExpr>>,
     distinct: bool,
 ) -> Result<Arc<Expr>> {
     let args =
         from_substrait_func_args(state, &f.arguments, input_schema, extensions).await?;
 
-    let Some(function_name) = extensions.functions.get(&f.function_reference) else {
-        return plan_err!(
-            "Aggregate function not registered: function anchor = {:?}",
-            f.function_reference
-        );
-    };
-
-    let function_name = substrait_fun_name(function_name);
     // try udaf first, then built-in aggr fn.
     if let Ok(fun) = state.udaf(function_name) {
         // deal with situation that count(*) got no arguments
@@ -1530,7 +1541,15 @@ pub async fn from_substrait_agg_func(
         };
 
         Ok(Arc::new(Expr::AggregateFunction(
-            expr::AggregateFunction::new_udf(fun, args, distinct, filter, order_by, None),
+            expr::AggregateFunction::new_udf(
+                fun,
+                args,
+                distinct,
+                filter,
+                order_by,
+                None,
+                within_group,
+            ),
         )))
     } else {
         not_impl_err!(
diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs
index 29019dfd74f32..f86f105cdbe5d 100644
--- a/datafusion/substrait/src/logical_plan/producer.rs
+++ b/datafusion/substrait/src/logical_plan/producer.rs
@@ -869,12 +869,23 @@ pub fn to_substrait_agg_measure(
     extensions: &mut Extensions,
 ) -> Result<Measure> {
     match expr {
-        Expr::AggregateFunction(expr::AggregateFunction { func, args, distinct, filter, order_by, null_treatment: _, }) => {
-                    let sorts = if let Some(order_by) = order_by {
-                        order_by.iter().map(|expr| to_substrait_sort_field(state, expr, schema, extensions)).collect::<Result<Vec<_>>>()?
-                    } else {
-                        vec![]
+        Expr::AggregateFunction(expr::AggregateFunction { func, args, distinct, filter, order_by, null_treatment: _, within_group}) => {
+                    let sorts = match (within_group, order_by) {
+                        (Some(within_group), _) => {
+                            within_group
+                                .iter()
+                                .map(|expr| to_substrait_sort_field(state, expr, schema, extensions))
+                                .collect::<Result<Vec<_>>>()?
+                        }
+                        (None, Some(order_by)) => {
+                            order_by
+                                .iter()
+                                .map(|expr| to_substrait_sort_field(state, expr, schema, extensions))
+                                .collect::<Result<Vec<_>>>()?
+                        }
+                        (None, None) => vec![],
                     };
+
                     let mut arguments: Vec<FunctionArgument> = vec![];
                     for arg in args {
                         arguments.push(FunctionArgument { arg_type: Some(ArgType::Value(to_substrait_rex(state, arg, schema, 0, extensions)?)) });

From c3be3c66f38d26be46efe6d9eb49b2c3eaceb800 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Tue, 21 Jan 2025 17:16:15 +0900
Subject: [PATCH 05/39] Modify physical planner and accumulator to support
 ordered set aggregate function

---
 datafusion/core/src/physical_planner.rs          | 16 ++++++++++++----
 .../src/approx_percentile_cont.rs                | 13 +++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs
index 44537c951f945..bf69de9227f8e 100644
--- a/datafusion/core/src/physical_planner.rs
+++ b/datafusion/core/src/physical_planner.rs
@@ -1594,6 +1594,7 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
             filter,
             order_by,
             null_treatment,
+            within_group,
         }) => {
             let name = if let Some(name) = name {
                 name
@@ -1616,13 +1617,20 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
                 == NullTreatment::IgnoreNulls;
 
             let (agg_expr, filter, order_by) = {
-                let physical_sort_exprs = match order_by {
-                    Some(exprs) => Some(create_physical_sort_exprs(
-                        exprs,
+                let physical_sort_exprs = match within_group {
+                    Some(within_group) => Some(create_physical_sort_exprs(
+                        within_group,
                         logical_input_schema,
                         execution_props,
                     )?),
-                    None => None,
+                    None => match order_by {
+                        Some(order_by) => Some(create_physical_sort_exprs(
+                            order_by,
+                            logical_input_schema,
+                            execution_props,
+                        )?),
+                        None => None,
+                    },
                 };
 
                 let ordering_reqs: LexOrdering =
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 3068508c350f1..0787506222f1c 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -124,6 +124,19 @@ impl ApproxPercentileCont {
         args: AccumulatorArgs,
     ) -> Result<ApproxPercentileAccumulator> {
         let percentile = validate_input_percentile_expr(&args.exprs[1])?;
+
+        let is_descending = args
+            .ordering_req
+            .first()
+            .map(|sort_expr| sort_expr.options.descending)
+            .unwrap_or(false);
+
+        let percentile = if is_descending {
+            1.0 - percentile
+        } else {
+            percentile
+        };
+
         let tdigest_max_size = if args.exprs.len() == 3 {
             Some(validate_input_max_size_expr(&args.exprs[2])?)
         } else {

From 9fd05a35429beac4892832d94c535b56c3abddd8 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 23 Jan 2025 16:24:41 +0900
Subject: [PATCH 06/39] Support session management for ordered set aggregate
 functions

---
 datafusion/catalog/src/session.rs             |  6 ++
 datafusion/core/src/execution/context/mod.rs  | 13 +++
 .../core/src/execution/session_state.rs       | 94 +++++++++++++++++++
 .../src/execution/session_state_defaults.rs   |  7 ++
 datafusion/core/tests/optimizer/mod.rs        |  4 +
 datafusion/execution/src/task.rs              | 32 +++++++
 datafusion/expr/src/planner.rs                |  4 +
 datafusion/expr/src/registry.rs               | 53 +++++++++++
 datafusion/functions-aggregate/src/lib.rs     | 10 ++
 datafusion/proto/src/bytes/mod.rs             | 19 ++++
 datafusion/proto/src/bytes/registry.rs        | 10 ++
 11 files changed, 252 insertions(+)

diff --git a/datafusion/catalog/src/session.rs b/datafusion/catalog/src/session.rs
index db49529ac43f5..373866b61a4fe 100644
--- a/datafusion/catalog/src/session.rs
+++ b/datafusion/catalog/src/session.rs
@@ -110,6 +110,11 @@ pub trait Session: Send + Sync {
     /// Return reference to aggregate_functions
     fn aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>>;
 
+    /// Return reference to ordered_set_aggregate_functions
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>>;
+
     /// Return reference to window functions
     fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>>;
 
@@ -132,6 +137,7 @@ impl From<&dyn Session> for TaskContext {
             state.config().clone(),
             state.scalar_functions().clone(),
             state.aggregate_functions().clone(),
+            state.ordered_set_aggregate_functions().clone(),
             state.window_functions().clone(),
             state.runtime_env().clone(),
         )
diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs
index 5f01d41c31e73..0824739c480af 100644
--- a/datafusion/core/src/execution/context/mod.rs
+++ b/datafusion/core/src/execution/context/mod.rs
@@ -1580,6 +1580,10 @@ impl FunctionRegistry for SessionContext {
         self.state.read().udaf(name)
     }
 
+    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
+        self.state.read().ordered_set_udaf(name)
+    }
+
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         self.state.read().udwf(name)
     }
@@ -1595,6 +1599,15 @@ impl FunctionRegistry for SessionContext {
         self.state.write().register_udaf(udaf)
     }
 
+    fn register_ordered_set_udaf(
+        &mut self,
+        ordered_set_udaf: Arc<AggregateUDF>,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        self.state
+            .write()
+            .register_ordered_set_udaf(ordered_set_udaf)
+    }
+
     fn register_udwf(&mut self, udwf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         self.state.write().register_udwf(udwf)
     }
diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs
index e99cf82223815..b7da115d56fb5 100644
--- a/datafusion/core/src/execution/session_state.rs
+++ b/datafusion/core/src/execution/session_state.rs
@@ -144,6 +144,10 @@ pub struct SessionState {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate functions registered in the context
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+    /// Ordered-set aggregate functions registered in the context
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     /// Window functions registered in the context
     window_functions: HashMap<String, Arc<WindowUDF>>,
     /// Deserializer registry for extensions.
@@ -202,6 +206,7 @@ impl Debug for SessionState {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
+            .field("ordered_set_aggregate_functions", &self.ordered_set_aggregate_functions)
             .field("window_functions", &self.window_functions)
             .field("prepared_plans", &self.prepared_plans)
             .finish()
@@ -241,6 +246,10 @@ impl Session for SessionState {
         &self.aggregate_functions
     }
 
+    fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
+        &self.ordered_set_aggregate_functions
+    }
+
     fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
     }
@@ -880,6 +889,13 @@ impl SessionState {
         &self.aggregate_functions
     }
 
+    /// Return reference to ordered_set_aggregate_functions
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    pub fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
+        &self.ordered_set_aggregate_functions
+    }
+
     /// Return reference to window functions
     pub fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
@@ -968,6 +984,7 @@ pub struct SessionStateBuilder {
     table_functions: Option<HashMap<String, Arc<TableFunction>>>,
     scalar_functions: Option<Vec<Arc<ScalarUDF>>>,
     aggregate_functions: Option<Vec<Arc<AggregateUDF>>>,
+    ordered_set_aggregate_functions: Option<Vec<Arc<AggregateUDF>>>,
     window_functions: Option<Vec<Arc<WindowUDF>>>,
     serializer_registry: Option<Arc<dyn SerializerRegistry>>,
     file_formats: Option<Vec<Arc<dyn FileFormatFactory>>>,
@@ -998,6 +1015,7 @@ impl SessionStateBuilder {
             table_functions: None,
             scalar_functions: None,
             aggregate_functions: None,
+            ordered_set_aggregate_functions: None,
             window_functions: None,
             serializer_registry: None,
             file_formats: None,
@@ -1048,6 +1066,9 @@ impl SessionStateBuilder {
             aggregate_functions: Some(
                 existing.aggregate_functions.into_values().collect_vec(),
             ),
+            ordered_set_aggregate_functions: Some(
+                existing.ordered_set_aggregate_functions.into_values().collect_vec(),
+            ),
             window_functions: Some(existing.window_functions.into_values().collect_vec()),
             serializer_registry: Some(existing.serializer_registry),
             file_formats: Some(existing.file_formats.into_values().collect_vec()),
@@ -1073,6 +1094,7 @@ impl SessionStateBuilder {
             .with_expr_planners(SessionStateDefaults::default_expr_planners())
             .with_scalar_functions(SessionStateDefaults::default_scalar_functions())
             .with_aggregate_functions(SessionStateDefaults::default_aggregate_functions())
+            .with_ordered_set_aggregate_functions(SessionStateDefaults::default_ordered_set_aggregate_functions())
             .with_window_functions(SessionStateDefaults::default_window_functions())
     }
 
@@ -1206,6 +1228,17 @@ impl SessionStateBuilder {
         self
     }
 
+    /// Set the map of ordered-set [`AggregateUDF`]s
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    pub fn with_ordered_set_aggregate_functions(
+        mut self,
+        ordered_set_aggregate_functions: Vec<Arc<AggregateUDF>>,
+    ) -> Self {
+        self.ordered_set_aggregate_functions = Some(ordered_set_aggregate_functions);
+        self
+    }
+
     /// Set the map of [`WindowUDF`]s
     pub fn with_window_functions(
         mut self,
@@ -1340,6 +1373,7 @@ impl SessionStateBuilder {
             table_functions,
             scalar_functions,
             aggregate_functions,
+            ordered_set_aggregate_functions,
             window_functions,
             serializer_registry,
             file_formats,
@@ -1371,6 +1405,7 @@ impl SessionStateBuilder {
             table_functions: table_functions.unwrap_or_default(),
             scalar_functions: HashMap::new(),
             aggregate_functions: HashMap::new(),
+            ordered_set_aggregate_functions: HashMap::new(),
             window_functions: HashMap::new(),
             serializer_registry: serializer_registry
                 .unwrap_or(Arc::new(EmptySerializerRegistry)),
@@ -1411,6 +1446,20 @@ impl SessionStateBuilder {
             });
         }
 
+        if let Some(ordered_set_aggregate_functions) = ordered_set_aggregate_functions {
+            ordered_set_aggregate_functions
+                .into_iter()
+                .for_each(|udaf| {
+                    let existing_ordered_set_udf = state.register_ordered_set_udaf(udaf);
+                    if let Ok(Some(existing_ordered_set_udf)) = existing_ordered_set_udf {
+                        debug!(
+                            "Overwrote an ordered set existing UDF: {}",
+                            existing_ordered_set_udf.name()
+                        );
+                    }
+                });
+        }
+
         if let Some(window_functions) = window_functions {
             window_functions.into_iter().for_each(|udwf| {
                 let existing_udf = state.register_udwf(udwf);
@@ -1610,6 +1659,7 @@ impl Debug for SessionStateBuilder {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
+            .field("ordered_set_aggregate_functions", &self.ordered_set_aggregate_functions)
             .field("window_functions", &self.window_functions)
             .finish()
     }
@@ -1696,6 +1746,13 @@ impl<'a> ContextProvider for SessionContextProvider<'a> {
         self.state.aggregate_functions().get(name).cloned()
     }
 
+    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
+        self.state
+            .ordered_set_aggregate_functions()
+            .get(name)
+            .cloned()
+    }
+
     fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>> {
         self.state.window_functions().get(name).cloned()
     }
@@ -1766,6 +1823,17 @@ impl FunctionRegistry for SessionState {
         })
     }
 
+    fn ordered_set_udaf(
+        &self,
+        name: &str,
+    ) -> datafusion_common::Result<Arc<AggregateUDF>> {
+        let result = self.ordered_set_aggregate_functions.get(name);
+
+        result.cloned().ok_or_else(|| {
+            plan_datafusion_err!("There is no ordered set UDAF named \"{name}\" in the registry")
+        })
+    }
+
     fn udwf(&self, name: &str) -> datafusion_common::Result<Arc<WindowUDF>> {
         let result = self.window_functions.get(name);
 
@@ -1796,6 +1864,18 @@ impl FunctionRegistry for SessionState {
         Ok(self.aggregate_functions.insert(udaf.name().into(), udaf))
     }
 
+    fn register_ordered_set_udaf(
+        &mut self,
+        ordered_set_udaf: Arc<AggregateUDF>,
+    ) -> datafusion_common::Result<Option<Arc<AggregateUDF>>> {
+        ordered_set_udaf.aliases().iter().for_each(|alias| {
+            self.ordered_set_aggregate_functions
+                .insert(alias.clone(), Arc::clone(&ordered_set_udaf));
+        });
+        Ok(self.ordered_set_aggregate_functions
+            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
+    }
+
     fn register_udwf(
         &mut self,
         udwf: Arc<WindowUDF>,
@@ -1833,6 +1913,19 @@ impl FunctionRegistry for SessionState {
         Ok(udaf)
     }
 
+    fn deregister_ordered_set_udaf(
+        &mut self,
+        name: &str,
+    ) -> datafusion_common::Result<Option<Arc<AggregateUDF>>> {
+        let ordered_set_udaf = self.ordered_set_aggregate_functions.remove(name);
+        if let Some(ordered_set_udaf) = &ordered_set_udaf {
+            for alias in ordered_set_udaf.aliases() {
+                self.ordered_set_aggregate_functions.remove(alias);
+            }
+        }
+        Ok(ordered_set_udaf)
+    }
+
     fn deregister_udwf(
         &mut self,
         name: &str,
@@ -1895,6 +1988,7 @@ impl From<&SessionState> for TaskContext {
             state.config.clone(),
             state.scalar_functions.clone(),
             state.aggregate_functions.clone(),
+            state.ordered_set_aggregate_functions.clone(),
             state.window_functions.clone(),
             Arc::clone(&state.runtime_env),
         )
diff --git a/datafusion/core/src/execution/session_state_defaults.rs b/datafusion/core/src/execution/session_state_defaults.rs
index 7ba332c520c1a..604b3e936e097 100644
--- a/datafusion/core/src/execution/session_state_defaults.rs
+++ b/datafusion/core/src/execution/session_state_defaults.rs
@@ -114,6 +114,13 @@ impl SessionStateDefaults {
         functions_aggregate::all_default_aggregate_functions()
     }
 
+    /// returns the list of default ordered-set [`AggregateUDF']'s
+    ///
+    /// Note : default_ordered_set_aggregate_functions are a subset of default_aggregate_functions.
+    pub fn default_ordered_set_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
+        functions_aggregate::all_default_ordered_set_aggregate_functions()
+    }
+
     /// returns the list of default [`WindowUDF']'s
     pub fn default_window_functions() -> Vec<Arc<WindowUDF>> {
         functions_window::all_default_window_functions()
diff --git a/datafusion/core/tests/optimizer/mod.rs b/datafusion/core/tests/optimizer/mod.rs
index f17d13a420607..86c8e02a6c811 100644
--- a/datafusion/core/tests/optimizer/mod.rs
+++ b/datafusion/core/tests/optimizer/mod.rs
@@ -202,6 +202,10 @@ impl ContextProvider for MyContextProvider {
         None
     }
 
+    fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
+        None
+    }
+
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/execution/src/task.rs b/datafusion/execution/src/task.rs
index 35494443b4760..d66ac5717d3c8 100644
--- a/datafusion/execution/src/task.rs
+++ b/datafusion/execution/src/task.rs
@@ -46,6 +46,10 @@ pub struct TaskContext {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate functions associated with this task context
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+    /// Ordered set aggregate functions associated with this task context
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     /// Window functions associated with this task context
     window_functions: HashMap<String, Arc<WindowUDF>>,
     /// Runtime environment associated with this task context
@@ -65,6 +69,7 @@ impl Default for TaskContext {
             session_config: SessionConfig::new(),
             scalar_functions: HashMap::new(),
             aggregate_functions: HashMap::new(),
+            ordered_set_aggregate_functions: HashMap::new(),
             window_functions: HashMap::new(),
             runtime,
         }
@@ -83,6 +88,7 @@ impl TaskContext {
         session_config: SessionConfig,
         scalar_functions: HashMap<String, Arc<ScalarUDF>>,
         aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+        ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
         window_functions: HashMap<String, Arc<WindowUDF>>,
         runtime: Arc<RuntimeEnv>,
     ) -> Self {
@@ -92,6 +98,7 @@ impl TaskContext {
             session_config,
             scalar_functions,
             aggregate_functions,
+            ordered_set_aggregate_functions,
             window_functions,
             runtime,
         }
@@ -130,6 +137,10 @@ impl TaskContext {
         &self.aggregate_functions
     }
 
+    pub fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
+        &self.ordered_set_aggregate_functions
+    }
+
     pub fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
     }
@@ -168,6 +179,14 @@ impl FunctionRegistry for TaskContext {
         })
     }
 
+    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
+        let result = self.ordered_set_aggregate_functions.get(name);
+
+        result.cloned().ok_or_else(|| {
+            plan_datafusion_err!("There is no ordered set UDAF named \"{name}\" in the TaskContext")
+        })
+    }
+
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         let result = self.window_functions.get(name);
 
@@ -187,6 +206,18 @@ impl FunctionRegistry for TaskContext {
         });
         Ok(self.aggregate_functions.insert(udaf.name().into(), udaf))
     }
+    fn register_ordered_set_udaf(
+        &mut self,
+        ordered_set_udaf: Arc<AggregateUDF>,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        ordered_set_udaf.aliases().iter().for_each(|alias| {
+            self.ordered_set_aggregate_functions
+                .insert(alias.clone(), Arc::clone(&ordered_set_udaf));
+        });
+        Ok(self
+            .ordered_set_aggregate_functions
+            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
+    }
     fn register_udwf(&mut self, udwf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         udwf.aliases().iter().for_each(|alias| {
             self.window_functions
@@ -242,6 +273,7 @@ mod tests {
             HashMap::default(),
             HashMap::default(),
             HashMap::default(),
+            HashMap::default(),
             runtime,
         );
 
diff --git a/datafusion/expr/src/planner.rs b/datafusion/expr/src/planner.rs
index 42047e8e6caa2..fa4a4f5025f7a 100644
--- a/datafusion/expr/src/planner.rs
+++ b/datafusion/expr/src/planner.rs
@@ -76,6 +76,10 @@ pub trait ContextProvider {
     fn get_function_meta(&self, name: &str) -> Option<Arc<ScalarUDF>>;
     /// Getter for a UDAF description
     fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
+    /// Getter for a ordered-set UDAF description
+    ///
+    /// Note : ordered_set_aggregate_meta is a subset of aggregate_meta.
+    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
     /// Getter for a UDWF
     fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>>;
     /// Getter for system/user-defined variable type
diff --git a/datafusion/expr/src/registry.rs b/datafusion/expr/src/registry.rs
index 4eb49710bcf85..4f43289300a7d 100644
--- a/datafusion/expr/src/registry.rs
+++ b/datafusion/expr/src/registry.rs
@@ -38,6 +38,12 @@ pub trait FunctionRegistry {
     /// `name`.
     fn udaf(&self, name: &str) -> Result<Arc<AggregateUDF>>;
 
+    /// Returns a reference to the user defined ordered set aggregate function (udaf) named
+    /// `name`.
+    ///
+    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
+    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>>;
+
     /// Returns a reference to the user defined window function (udwf) named
     /// `name`.
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>>;
@@ -61,6 +67,21 @@ pub trait FunctionRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         not_impl_err!("Registering AggregateUDF")
     }
+
+    /// Registers a new ordered-set [`AggregateUDF`], returning any previously registered
+    /// implementation.
+    ///
+    /// Returns an error (the default) if the function can not be registered,
+    /// for example if the registry is read only.
+    ///
+    /// Note : ordered_set_udaf is a subset of udaf.
+    fn register_ordered_set_udaf(
+        &mut self,
+        _udaf: Arc<AggregateUDF>,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        not_impl_err!("Registering ordered-set AggregateUDF")
+    }
+
     /// Registers a new [`WindowUDF`], returning any previously registered
     /// implementation.
     ///
@@ -88,6 +109,20 @@ pub trait FunctionRegistry {
         not_impl_err!("Deregistering AggregateUDF")
     }
 
+    /// Deregisters a ordered set [`AggregateUDF`], returning the implementation that was
+    /// deregistered.
+    ///
+    /// Returns an error (the default) if the function can not be deregistered,
+    /// for example if the registry is read only.
+    ///
+    /// Note : ordered_set_udaf is a subset of udaf.
+    fn deregister_ordered_set_udaf(
+        &mut self,
+        _name: &str,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        not_impl_err!("Deregistering ordered set AggregateUDF")
+    }
+
     /// Deregisters a [`WindowUDF`], returning the implementation that was
     /// deregistered.
     ///
@@ -148,6 +183,10 @@ pub struct MemoryFunctionRegistry {
     udfs: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate Functions
     udafs: HashMap<String, Arc<AggregateUDF>>,
+    /// Ordered Set Aggregate Functions
+    ///
+    /// Note : ordered_set_udafs are a subset of udafs.
+    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
     /// Window Functions
     udwfs: HashMap<String, Arc<WindowUDF>>,
 }
@@ -177,6 +216,13 @@ impl FunctionRegistry for MemoryFunctionRegistry {
             .ok_or_else(|| plan_datafusion_err!("Aggregate Function {name} not found"))
     }
 
+    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
+        self.ordered_set_udafs
+            .get(name)
+            .cloned()
+            .ok_or_else(|| plan_datafusion_err!("Ordered Set Aggregate Function {name} not found"))
+    }
+
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         self.udwfs
             .get(name)
@@ -193,6 +239,13 @@ impl FunctionRegistry for MemoryFunctionRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         Ok(self.udafs.insert(udaf.name().into(), udaf))
     }
+    fn register_ordered_set_udaf(
+        &mut self,
+        ordered_set_udaf: Arc<AggregateUDF>,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        Ok(self.ordered_set_udafs
+            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
+    }
     fn register_udwf(&mut self, udaf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         Ok(self.udwfs.insert(udaf.name().into(), udaf))
     }
diff --git a/datafusion/functions-aggregate/src/lib.rs b/datafusion/functions-aggregate/src/lib.rs
index 746873442d9aa..b0e9079b69ac5 100644
--- a/datafusion/functions-aggregate/src/lib.rs
+++ b/datafusion/functions-aggregate/src/lib.rs
@@ -174,6 +174,16 @@ pub fn all_default_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
     ]
 }
 
+/// Returns all default ordered set aggregate functions
+///
+/// Note : all_default_ordered_set_aggregate_functions are a subset of all_default_aggregate_functions.
+pub fn all_default_ordered_set_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
+    vec![
+        approx_percentile_cont_udaf(),
+        approx_percentile_cont_with_weight_udaf(),
+    ]
+}
+
 /// Registers all enabled packages with a [`FunctionRegistry`]
 pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
     let functions: Vec<Arc<AggregateUDF>> = all_default_aggregate_functions();
diff --git a/datafusion/proto/src/bytes/mod.rs b/datafusion/proto/src/bytes/mod.rs
index 12ddb4cb2e329..2d56c86975846 100644
--- a/datafusion/proto/src/bytes/mod.rs
+++ b/datafusion/proto/src/bytes/mod.rs
@@ -133,6 +133,17 @@ impl Serializeable for Expr {
                 )))
             }
 
+            fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
+                Ok(Arc::new(create_udaf(
+                    name,
+                    vec![arrow::datatypes::DataType::Null],
+                    Arc::new(arrow::datatypes::DataType::Null),
+                    Volatility::Immutable,
+                    Arc::new(|_| unimplemented!()),
+                    Arc::new(vec![]),
+                )))
+            }
+
             fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
                 Ok(Arc::new(create_udwf(
                     name,
@@ -150,6 +161,14 @@ impl Serializeable for Expr {
                     "register_udaf called in Placeholder Registry!"
                 )
             }
+            fn register_ordered_set_udaf(
+                &mut self,
+                _udaf: Arc<AggregateUDF>,
+            ) -> Result<Option<Arc<AggregateUDF>>> {
+                datafusion_common::internal_err!(
+                    "register_ordered_set_udaf called in Placeholder Registry!"
+                )
+            }
             fn register_udf(
                 &mut self,
                 _udf: Arc<datafusion_expr::ScalarUDF>,
diff --git a/datafusion/proto/src/bytes/registry.rs b/datafusion/proto/src/bytes/registry.rs
index eae2425f8ac19..d14f58b651f7c 100644
--- a/datafusion/proto/src/bytes/registry.rs
+++ b/datafusion/proto/src/bytes/registry.rs
@@ -40,6 +40,10 @@ impl FunctionRegistry for NoRegistry {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Aggregate Function '{name}'")
     }
 
+    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
+        plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Ordered Set Aggregate Function '{name}'")
+    }
+
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Window Function '{name}'")
     }
@@ -49,6 +53,12 @@ impl FunctionRegistry for NoRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         plan_err!("No function registry provided to deserialize, so can not register User Defined Aggregate Function '{}'", udaf.inner().name())
     }
+    fn register_ordered_set_udaf(
+        &mut self,
+        ordered_set_udaf: Arc<AggregateUDF>,
+    ) -> Result<Option<Arc<AggregateUDF>>> {
+        plan_err!("No function registry provided to deserialize, so can not register User Defined Ordered Set Aggregate Function '{}'", ordered_set_udaf.inner().name())
+    }
     fn register_udf(&mut self, udf: Arc<ScalarUDF>) -> Result<Option<Arc<ScalarUDF>>> {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Function '{}'", udf.inner().name())
     }

From 8518a59b3b4f8bceac07798d5deab2aa63a2a590 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 25 Jan 2025 21:51:05 +0900
Subject: [PATCH 07/39] Align code, tests, and examples with changes to
 aggregate function logic

* Ensure compatibility with new `within_group` and `order_by` handling.

* Adjust tests and examples to align with the new logic.
---
 .../examples/simplify_udaf_expression.rs      |   1 +
 datafusion-examples/examples/sql_frontend.rs  |   4 +
 datafusion/expr/src/test/function_stub.rs     |   5 +
 datafusion/expr/src/tree_node.rs              |  44 +++++---
 datafusion/functions-aggregate/src/count.rs   |   1 +
 datafusion/functions-nested/src/planner.rs    |   1 +
 .../optimizer/src/analyzer/type_coercion.rs   |   7 ++
 .../optimizer/src/common_subexpr_eliminate.rs |   1 +
 .../simplify_expressions/expr_simplifier.rs   |   2 +
 .../src/single_distinct_to_groupby.rs         |   9 +-
 .../optimizer/tests/optimizer_integration.rs  |   5 +
 .../tests/cases/roundtrip_logical_plan.rs     |   1 +
 datafusion/sql/examples/sql.rs                |   6 ++
 datafusion/sql/src/expr/mod.rs                |   4 +
 datafusion/sql/tests/common/mod.rs            |   5 +
 .../sqllogictest/test_files/aggregate.slt     | 102 +++++++++---------
 16 files changed, 135 insertions(+), 63 deletions(-)

diff --git a/datafusion-examples/examples/simplify_udaf_expression.rs b/datafusion-examples/examples/simplify_udaf_expression.rs
index 52a27317e3c3d..2fadc6e2e5e91 100644
--- a/datafusion-examples/examples/simplify_udaf_expression.rs
+++ b/datafusion-examples/examples/simplify_udaf_expression.rs
@@ -100,6 +100,7 @@ impl AggregateUDFImpl for BetterAvgUdaf {
                 aggregate_function.filter,
                 aggregate_function.order_by,
                 aggregate_function.null_treatment,
+                aggregate_function.within_group,
             )))
         };
 
diff --git a/datafusion-examples/examples/sql_frontend.rs b/datafusion-examples/examples/sql_frontend.rs
index 839ee95eb1816..543b5009b6868 100644
--- a/datafusion-examples/examples/sql_frontend.rs
+++ b/datafusion-examples/examples/sql_frontend.rs
@@ -157,6 +157,10 @@ impl ContextProvider for MyContextProvider {
         None
     }
 
+    fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
+        None
+    }
+
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/expr/src/test/function_stub.rs b/datafusion/expr/src/test/function_stub.rs
index 262aa99e50075..2b837a8c7f7e9 100644
--- a/datafusion/expr/src/test/function_stub.rs
+++ b/datafusion/expr/src/test/function_stub.rs
@@ -67,6 +67,7 @@ pub fn sum(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
@@ -80,6 +81,7 @@ pub fn count(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
@@ -93,6 +95,7 @@ pub fn avg(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
@@ -291,6 +294,7 @@ pub fn min(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
@@ -376,6 +380,7 @@ pub fn max(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs
index eacace5ed0461..5ab69d3da4c8d 100644
--- a/datafusion/expr/src/tree_node.rs
+++ b/datafusion/expr/src/tree_node.rs
@@ -247,18 +247,38 @@ impl TreeNode for Expr {
                 filter,
                 order_by,
                 null_treatment,
-            }) => (args, filter, order_by).map_elements(f)?.map_data(
-                |(new_args, new_filter, new_order_by)| {
-                    Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
-                        func,
-                        new_args,
-                        distinct,
-                        new_filter,
-                        new_order_by,
-                        null_treatment,
-                    )))
-                },
-            )?,
+                within_group,
+            }) => {
+                if within_group.is_some() {
+                    (args, filter, within_group).map_elements(f)?.map_data(
+                        |(new_args, new_filter, new_within_group)| {
+                            Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
+                                func,
+                                new_args,
+                                distinct,
+                                new_filter,
+                                order_by,
+                                null_treatment,
+                                new_within_group,
+                            )))
+                        },
+                    )?
+                } else {
+                    (args, filter, order_by).map_elements(f)?.map_data(
+                        |(new_args, new_filter, new_order_by)| {
+                            Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
+                                func,
+                                new_args,
+                                distinct,
+                                new_filter,
+                                new_order_by,
+                                null_treatment,
+                                within_group,
+                            )))
+                        },
+                    )?
+                }
+            },
             Expr::GroupingSet(grouping_set) => match grouping_set {
                 GroupingSet::Rollup(exprs) => exprs
                     .map_elements(f)?
diff --git a/datafusion/functions-aggregate/src/count.rs b/datafusion/functions-aggregate/src/count.rs
index 8fdd702b5b7c6..ef8a148c60f27 100644
--- a/datafusion/functions-aggregate/src/count.rs
+++ b/datafusion/functions-aggregate/src/count.rs
@@ -76,6 +76,7 @@ pub fn count_distinct(expr: Expr) -> Expr {
         None,
         None,
         None,
+        None,
     ))
 }
 
diff --git a/datafusion/functions-nested/src/planner.rs b/datafusion/functions-nested/src/planner.rs
index 1929b8222a1b6..c27a9e3e0dd37 100644
--- a/datafusion/functions-nested/src/planner.rs
+++ b/datafusion/functions-nested/src/planner.rs
@@ -163,6 +163,7 @@ impl ExprPlanner for FieldAccessPlanner {
                                 agg_func.filter,
                                 agg_func.order_by,
                                 agg_func.null_treatment,
+                                agg_func.within_group,
                             ),
                         )))
                     }
diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs
index b56c2dc604a9b..38768fcb2b928 100644
--- a/datafusion/optimizer/src/analyzer/type_coercion.rs
+++ b/datafusion/optimizer/src/analyzer/type_coercion.rs
@@ -500,6 +500,7 @@ impl<'a> TreeNodeRewriter for TypeCoercionRewriter<'a> {
                 filter,
                 order_by,
                 null_treatment,
+                within_group,
             }) => {
                 let new_expr = coerce_arguments_for_signature_with_aggregate_udf(
                     args,
@@ -514,6 +515,7 @@ impl<'a> TreeNodeRewriter for TypeCoercionRewriter<'a> {
                         filter,
                         order_by,
                         null_treatment,
+                        within_group,
                     ),
                 )))
             }
@@ -1319,6 +1321,7 @@ mod test {
             None,
             None,
             None,
+            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![udaf], empty)?);
         let expected = "Projection: MY_AVG(CAST(Int64(10) AS Float64))\n  EmptyRelation";
@@ -1348,6 +1351,7 @@ mod test {
             None,
             None,
             None,
+            None,
         ));
 
         let err = Projection::try_new(vec![udaf], empty).err().unwrap();
@@ -1367,6 +1371,7 @@ mod test {
             None,
             None,
             None,
+            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![agg_expr], empty)?);
         let expected = "Projection: avg(Float64(12))\n  EmptyRelation";
@@ -1380,6 +1385,7 @@ mod test {
             None,
             None,
             None,
+            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![agg_expr], empty)?);
         let expected = "Projection: avg(CAST(a AS Float64))\n  EmptyRelation";
@@ -1397,6 +1403,7 @@ mod test {
             None,
             None,
             None,
+            None,
         ));
         let err = Projection::try_new(vec![agg_expr], empty)
             .err()
diff --git a/datafusion/optimizer/src/common_subexpr_eliminate.rs b/datafusion/optimizer/src/common_subexpr_eliminate.rs
index 16a4fa6be38d0..7681ea493754c 100644
--- a/datafusion/optimizer/src/common_subexpr_eliminate.rs
+++ b/datafusion/optimizer/src/common_subexpr_eliminate.rs
@@ -891,6 +891,7 @@ mod test {
                 None,
                 None,
                 None,
+                None,
             ))
         };
 
diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
index 6564e722eaf89..dd14cd0c764dc 100644
--- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
+++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
@@ -3970,6 +3970,7 @@ mod tests {
                 None,
                 None,
                 None,
+                None,
             ));
 
         let expected = col("result_column");
@@ -3984,6 +3985,7 @@ mod tests {
                 None,
                 None,
                 None,
+                None,
             ));
 
         let expected = aggregate_function_expr.clone();
diff --git a/datafusion/optimizer/src/single_distinct_to_groupby.rs b/datafusion/optimizer/src/single_distinct_to_groupby.rs
index c8f3a4bc7859c..6fa60428cd80b 100644
--- a/datafusion/optimizer/src/single_distinct_to_groupby.rs
+++ b/datafusion/optimizer/src/single_distinct_to_groupby.rs
@@ -73,9 +73,10 @@ fn is_single_distinct_agg(aggr_expr: &[Expr]) -> Result<bool> {
             filter,
             order_by,
             null_treatment: _,
+            within_group,
         }) = expr
         {
-            if filter.is_some() || order_by.is_some() {
+            if filter.is_some() || order_by.is_some() || within_group.is_some() {
                 return Ok(false);
             }
             aggregate_count += 1;
@@ -200,6 +201,7 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                     None,
                                     None,
                                     None,
+                                    None,
                                 )))
                                 // if the aggregate function is not distinct, we need to rewrite it like two phase aggregation
                             } else {
@@ -213,6 +215,7 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                         None,
                                         None,
                                         None,
+                                        None,
                                     ))
                                     .alias(&alias_str),
                                 );
@@ -223,6 +226,7 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                     None,
                                     None,
                                     None,
+                                    None,
                                 )))
                             }
                         }
@@ -295,6 +299,7 @@ mod tests {
             None,
             None,
             None,
+            None,
         ))
     }
 
@@ -576,6 +581,7 @@ mod tests {
             Some(Box::new(col("a").gt(lit(5)))),
             None,
             None,
+            None,
         ));
         let plan = LogicalPlanBuilder::from(table_scan)
             .aggregate(vec![col("c")], vec![expr, count_distinct(col("b"))])?
@@ -619,6 +625,7 @@ mod tests {
             None,
             Some(vec![col("a").sort(true, false)]),
             None,
+            None,
         ));
         let plan = LogicalPlanBuilder::from(table_scan)
             .aggregate(vec![col("c")], vec![expr, count_distinct(col("b"))])?
diff --git a/datafusion/optimizer/tests/optimizer_integration.rs b/datafusion/optimizer/tests/optimizer_integration.rs
index 236167985790d..661ed4998ce8a 100644
--- a/datafusion/optimizer/tests/optimizer_integration.rs
+++ b/datafusion/optimizer/tests/optimizer_integration.rs
@@ -412,6 +412,7 @@ fn observe(_plan: &LogicalPlan, _rule: &dyn OptimizerRule) {}
 struct MyContextProvider {
     options: ConfigOptions,
     udafs: HashMap<String, Arc<AggregateUDF>>,
+    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
 }
 
 impl MyContextProvider {
@@ -465,6 +466,10 @@ impl ContextProvider for MyContextProvider {
         self.udafs.get(name).cloned()
     }
 
+    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
+        self.ordered_set_udafs.get(name).cloned()
+    }
+
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index 685dc834173df..d9c615606c77a 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -2202,6 +2202,7 @@ fn roundtrip_aggregate_udf() {
         Some(Box::new(lit(true))),
         None,
         None,
+        None,
     ));
 
     let ctx = SessionContext::new();
diff --git a/datafusion/sql/examples/sql.rs b/datafusion/sql/examples/sql.rs
index aa17be6273ae7..5966760fa103e 100644
--- a/datafusion/sql/examples/sql.rs
+++ b/datafusion/sql/examples/sql.rs
@@ -70,6 +70,7 @@ struct MyContextProvider {
     options: ConfigOptions,
     tables: HashMap<String, Arc<dyn TableSource>>,
     udafs: HashMap<String, Arc<AggregateUDF>>,
+    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
     expr_planners: Vec<Arc<dyn ExprPlanner>>,
 }
 
@@ -116,6 +117,7 @@ impl MyContextProvider {
             tables,
             options: Default::default(),
             udafs: Default::default(),
+            ordered_set_udafs: Default::default(),
             expr_planners: vec![],
         }
     }
@@ -143,6 +145,10 @@ impl ContextProvider for MyContextProvider {
         self.udafs.get(name).cloned()
     }
 
+    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
+        self.ordered_set_udafs.get(name).cloned()
+    }
+
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs
index 8c8d716a66657..cdad12afa9305 100644
--- a/datafusion/sql/src/expr/mod.rs
+++ b/datafusion/sql/src/expr/mod.rs
@@ -1095,6 +1095,10 @@ mod tests {
             None
         }
 
+        fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
+            None
+        }
+
         fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
             None
         }
diff --git a/datafusion/sql/tests/common/mod.rs b/datafusion/sql/tests/common/mod.rs
index 63c296dfbc2f9..ec99e6e8a7de2 100644
--- a/datafusion/sql/tests/common/mod.rs
+++ b/datafusion/sql/tests/common/mod.rs
@@ -54,6 +54,7 @@ impl Display for MockCsvType {
 pub(crate) struct MockSessionState {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     expr_planners: Vec<Arc<dyn ExprPlanner>>,
     type_planner: Option<Arc<dyn TypePlanner>>,
     window_functions: HashMap<String, Arc<WindowUDF>>,
@@ -227,6 +228,10 @@ impl ContextProvider for MockContextProvider {
         self.state.aggregate_functions.get(name).cloned()
     }
 
+    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
+        self.state.ordered_set_aggregate_functions.get(name).cloned()
+    }
+
     fn get_variable_type(&self, _: &[String]) -> Option<DataType> {
         unimplemented!()
     }
diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt
index 917e037682f24..ecae13885b832 100644
--- a/datafusion/sqllogictest/test_files/aggregate.slt
+++ b/datafusion/sqllogictest/test_files/aggregate.slt
@@ -77,36 +77,38 @@ SELECT approx_distinct(c9) count_c9, approx_distinct(cast(c9 as varchar)) count_
 
 # csv_query_approx_percentile_cont_with_weight
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Utf8, Int8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c1, c2, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c1) FROM aggregate_test_100
 
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Utf8, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c1, 0.95) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c1, 0.95) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
 
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Int8, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont_with_weight(c3, c2, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont_with_weight(c2, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
 
 # csv_query_approx_percentile_cont_with_histogram_bins
 statement error DataFusion error: External error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be UInt > 0 literal \(got data type Int64\)\.
-SELECT c1, approx_percentile_cont(c3, 0.95, -1000) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, -1000) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Utf8\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
 
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int16, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c3) FROM aggregate_test_100
 
 statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Float64, Float64, Float64\] to the signature OneOf(.*) failed(.|\n)*
-SELECT approx_percentile_cont(c12, 0.95, 111.1) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, 111.1) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
 
 statement error DataFusion error: This feature is not implemented: Percentile value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, c12) FROM aggregate_test_100
+SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
 
 statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
-SELECT approx_percentile_cont(c12, 0.95, c5) FROM aggregate_test_100
+SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
 
 # Not supported over sliding windows
 query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
-SELECT approx_percentile_cont(c3, 0.5) OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW) 
+SELECT approx_percentile_cont(0.5)
+WITHIN GROUP (ORDER BY c3)
+OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
 FROM aggregate_test_100
 
 # array agg can use order by
@@ -1177,173 +1179,173 @@ SELECT approx_distinct(c9) AS a, approx_distinct(c9) AS b FROM aggregate_test_10
 
 #csv_query_approx_percentile_cont (c2)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c2, 0.1) AS DOUBLE) / 1.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c2) AS DOUBLE) / 1.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c2, 0.5) AS DOUBLE) / 3.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c2) AS DOUBLE) / 3.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c2, 0.9) AS DOUBLE) / 5.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c2) AS DOUBLE) / 5.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c3)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c3, 0.1) AS DOUBLE) / -95.3) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c3) AS DOUBLE) / -95.3) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c3, 0.5) AS DOUBLE) / 15.5) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c3) AS DOUBLE) / 15.5) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c3, 0.9) AS DOUBLE) / 102.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c3) AS DOUBLE) / 102.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c4)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c4, 0.1) AS DOUBLE) / -22925.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c4) AS DOUBLE) / -22925.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c4, 0.5) AS DOUBLE) / 4599.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c4) AS DOUBLE) / 4599.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c4, 0.9) AS DOUBLE) / 25334.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c4) AS DOUBLE) / 25334.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c5)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c5, 0.1) AS DOUBLE) / -1882606710.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c5) AS DOUBLE) / -1882606710.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c5, 0.5) AS DOUBLE) / 377164262.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c5) AS DOUBLE) / 377164262.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c5, 0.9) AS DOUBLE) / 1991374996.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c5) AS DOUBLE) / 1991374996.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c6)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c6, 0.1) AS DOUBLE) / -7250000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c6) AS DOUBLE) / -7250000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c6, 0.5) AS DOUBLE) / 1130000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c6) AS DOUBLE) / 1130000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c6, 0.9) AS DOUBLE) / 7370000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c6) AS DOUBLE) / 7370000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c7)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c7, 0.1) AS DOUBLE) / 18.9) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c7) AS DOUBLE) / 18.9) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c7, 0.5) AS DOUBLE) / 134.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c7) AS DOUBLE) / 134.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c7, 0.9) AS DOUBLE) / 231.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c7) AS DOUBLE) / 231.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c8)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c8, 0.1) AS DOUBLE) / 2671.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c8) AS DOUBLE) / 2671.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c8, 0.5) AS DOUBLE) / 30634.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c8) AS DOUBLE) / 30634.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c8, 0.9) AS DOUBLE) / 57518.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c8) AS DOUBLE) / 57518.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c9)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c9, 0.1) AS DOUBLE) / 472608672.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c9) AS DOUBLE) / 472608672.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c9, 0.5) AS DOUBLE) / 2365817608.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c9) AS DOUBLE) / 2365817608.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c9, 0.9) AS DOUBLE) / 3776538487.0) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c9) AS DOUBLE) / 3776538487.0) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c10)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c10, 0.1) AS DOUBLE) / 1830000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c10) AS DOUBLE) / 1830000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c10, 0.5) AS DOUBLE) / 9300000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c10) AS DOUBLE) / 9300000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c10, 0.9) AS DOUBLE) / 16100000000000000000) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c10) AS DOUBLE) / 16100000000000000000) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # csv_query_approx_percentile_cont (c11)
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c11, 0.1) AS DOUBLE) /  0.109) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.1) WITHIN GROUP (ORDER BY c11) AS DOUBLE) /  0.109) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c11, 0.5) AS DOUBLE) / 0.491) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY c11) AS DOUBLE) / 0.491) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 query B
-SELECT (ABS(1 - CAST(approx_percentile_cont(c11, 0.9) AS DOUBLE) / 0.834) < 0.05) AS q FROM aggregate_test_100
+SELECT (ABS(1 - CAST(approx_percentile_cont(0.9) WITHIN GROUP (ORDER BY c11) AS DOUBLE) / 0.834) < 0.05) AS q FROM aggregate_test_100
 ----
 true
 
 # percentile_cont_with_nulls
 query I
-SELECT APPROX_PERCENTILE_CONT(v, 0.5) FROM (VALUES (1), (2), (3), (NULL), (NULL), (NULL)) as t (v);
+SELECT APPROX_PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v) FROM (VALUES (1), (2), (3), (NULL), (NULL), (NULL)) as t (v);
 ----
 2
 
 # percentile_cont_with_nulls_only
 query I
-SELECT APPROX_PERCENTILE_CONT(v, 0.5) FROM (VALUES (CAST(NULL as INT))) as t (v);
+SELECT APPROX_PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v) FROM (VALUES (CAST(NULL as INT))) as t (v);
 ----
 NULL
 
@@ -1366,7 +1368,7 @@ NaN
 
 # ISSUE: https://github.com/apache/datafusion/issues/11870
 query R
-select APPROX_PERCENTILE_CONT(v2, 0.8) from tmp_percentile_cont;
+select APPROX_PERCENTILE_CONT(0.8) WITHIN GROUP (ORDER BY v2) from tmp_percentile_cont;
 ----
 NaN
 
@@ -1374,10 +1376,10 @@ NaN
 # Note: `approx_percentile_cont_with_weight()` uses the same implementation as `approx_percentile_cont()`
 query R
 SELECT APPROX_PERCENTILE_CONT_WITH_WEIGHT(
-    v2,
     '+Inf'::Double,
     0.9
 )
+WITHIN GROUP (ORDER BY v2)
 FROM tmp_percentile_cont;
 ----
 NaN
@@ -1396,7 +1398,7 @@ INSERT INTO t1 VALUES (TRUE);
 # ISSUE: https://github.com/apache/datafusion/issues/12716
 # This test verifies that approx_percentile_cont_with_weight does not panic when given 'NaN' and returns 'inf'
 query R
-SELECT approx_percentile_cont_with_weight('NaN'::DOUBLE, 0, 0) FROM t1 WHERE t1.v1;
+SELECT approx_percentile_cont_with_weight(0, 0) WITHIN GROUP (ORDER BY 'NaN'::DOUBLE) FROM t1 WHERE t1.v1;
 ----
 Infinity
 
@@ -1623,7 +1625,7 @@ b NULL NULL 7732.315789473684
 
 # csv_query_approx_percentile_cont_with_weight
 query TI
-SELECT c1, approx_percentile_cont(c3, 0.95) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 ----
 a 73
 b 68
@@ -1633,7 +1635,7 @@ e 115
 
 # csv_query_approx_percentile_cont_with_weight (2)
 query TI
-SELECT c1, approx_percentile_cont_with_weight(c3, 1, 0.95) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont_with_weight(1, 0.95) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 ----
 a 73
 b 68
@@ -1643,7 +1645,7 @@ e 115
 
 # csv_query_approx_percentile_cont_with_histogram_bins
 query TI
-SELECT c1, approx_percentile_cont(c3, 0.95, 200) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(0.95, 200) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 ----
 a 73
 b 68
@@ -1652,7 +1654,7 @@ d 124
 e 115
 
 query TI
-SELECT c1, approx_percentile_cont_with_weight(c3, c2, 0.95) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont_with_weight(c2, 0.95) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 ----
 a 74
 b 68
@@ -2891,7 +2893,7 @@ SELECT COUNT(DISTINCT c1) FROM test
 
 # test_approx_percentile_cont_decimal_support
 query TI
-SELECT c1, approx_percentile_cont(c2, cast(0.85 as decimal(10,2))) apc FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+SELECT c1, approx_percentile_cont(cast(0.85 as decimal(10,2))) WITHIN GROUP (ORDER BY c2) apc FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
 ----
 a 4
 b 5

From 79669d9bc205c60a8eb277e5368ca9c255e56aa4 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 25 Jan 2025 21:59:09 +0900
Subject: [PATCH 08/39] Fix typo in existing comments

---
 datafusion/expr-common/src/accumulator.rs | 2 +-
 datafusion/expr/src/udaf.rs               | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/datafusion/expr-common/src/accumulator.rs b/datafusion/expr-common/src/accumulator.rs
index 7155c7993f8c9..51a26fa8c1ade 100644
--- a/datafusion/expr-common/src/accumulator.rs
+++ b/datafusion/expr-common/src/accumulator.rs
@@ -115,7 +115,7 @@ pub trait Accumulator: Send + Sync + Debug {
     ///              │                                │
     ///              │                                │
     /// ┌─────────────────────────┐      ┌─────────────────────────┐
-    /// │        GroubyBy         │      │        GroubyBy         │
+    /// │         GroupBy         │      │         GroupBy         │
     /// │(AggregateMode::Partial) │      │(AggregateMode::Partial) │
     /// └─────────────────────────┘      └─────────────────────────┘
     ///              ▲                                ▲
diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index 729c3be426f88..5d105e62a971f 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -399,7 +399,7 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
 
     /// Whether the aggregate function is nullable.
     ///
-    /// Nullable means that that the function could return `null` for any inputs.
+    /// Nullable means that the function could return `null` for any inputs.
     /// For example, aggregate functions like `COUNT` always return a non null value
     /// but others like `MIN` will return `NULL` if there is nullable input.
     /// Note that if the function is declared as *not* nullable, make sure the [`AggregateUDFImpl::default_value`] is `non-null`

From 597f4d77e3f953afef1f0f86e7733c6175525a10 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Mon, 27 Jan 2025 17:54:01 +0900
Subject: [PATCH 09/39] Enhance test

* Add test cases for changed signature

* Update signature in docs
---
 .../core/benches/aggregate_query_sql.rs       |  4 +--
 .../tests/dataframe/dataframe_functions.rs    | 30 ++++++++--------
 .../functions-aggregate/src/approx_median.rs  |  2 +-
 .../src/approx_percentile_cont.rs             | 14 ++++----
 .../src/approx_percentile_cont_with_weight.rs | 14 ++++----
 .../tests/cases/roundtrip_physical_plan.rs    |  2 +-
 datafusion/sql/src/expr/function.rs           |  8 ++---
 .../sqllogictest/test_files/aggregate.slt     | 12 +++++++
 .../user-guide/sql/aggregate_functions.md     | 34 +++++++++----------
 9 files changed, 66 insertions(+), 54 deletions(-)

diff --git a/datafusion/core/benches/aggregate_query_sql.rs b/datafusion/core/benches/aggregate_query_sql.rs
index 1d8d87ada7847..4ebe00af0a748 100644
--- a/datafusion/core/benches/aggregate_query_sql.rs
+++ b/datafusion/core/benches/aggregate_query_sql.rs
@@ -148,7 +148,7 @@ fn criterion_benchmark(c: &mut Criterion) {
         b.iter(|| {
             query(
                 ctx.clone(),
-                "SELECT utf8, approx_percentile_cont(u64_wide, 0.5, 2500)  \
+                "SELECT utf8, approx_percentile_cont(0.5, 2500) WITHIN GROUP (ORDER BY u64_wide)  \
                  FROM t GROUP BY utf8",
             )
         })
@@ -158,7 +158,7 @@ fn criterion_benchmark(c: &mut Criterion) {
         b.iter(|| {
             query(
                 ctx.clone(),
-                "SELECT utf8, approx_percentile_cont(f32, 0.5, 2500)  \
+                "SELECT utf8, approx_percentile_cont(0.5, 2500) WITHIN GROUP (ORDER BY f32)  \
                  FROM t GROUP BY utf8",
             )
         })
diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index fa37a03dfae4d..8871443e16ebf 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -363,11 +363,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
 
     let expected = [
-        "+-----------------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [b ASC NULLS LAST] |",
-        "+-----------------------------------------------------------------------------+",
-        "| 10                                                                          |",
-        "+-----------------------------------------------------------------------------+",
+        "+----------------------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+----------------------------------------------------------------------------------+",
+        "| 10                                                                               |",
+        "+----------------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
@@ -384,11 +384,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     let expr = approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
-        "+----------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,arg_2) WITHIN GROUP [b ASC NULLS LAST] |",
-        "+----------------------------------------------------------------------+",
-        "| 10                                                                   |",
-        "+----------------------------------------------------------------------+",
+        "+---------------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,arg_2) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+---------------------------------------------------------------------------+",
+        "| 10                                                                        |",
+        "+---------------------------------------------------------------------------+",
     ];
     let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
 
@@ -397,11 +397,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     // with number of centroids set
     let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), Some(lit(2)));
     let expected = [
-        "+--------------------------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [b ASC NULLS LAST] |",
-        "+--------------------------------------------------------------------------------------+",
-        "| 30                                                                                   |",
-        "+--------------------------------------------------------------------------------------+",
+        "+-------------------------------------------------------------------------------------------+",
+        "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+-------------------------------------------------------------------------------------------+",
+        "| 30                                                                                        |",
+        "+-------------------------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
diff --git a/datafusion/functions-aggregate/src/approx_median.rs b/datafusion/functions-aggregate/src/approx_median.rs
index 96609622a51e4..31f30353b9416 100644
--- a/datafusion/functions-aggregate/src/approx_median.rs
+++ b/datafusion/functions-aggregate/src/approx_median.rs
@@ -133,7 +133,7 @@ fn get_approx_median_doc() -> &'static Documentation {
         Documentation::builder()
             .with_doc_section(DOC_SECTION_APPROXIMATE)
             .with_description(
-                "Returns the approximate median (50th percentile) of input values. It is an alias of `approx_percentile_cont(x, 0.5)`.",
+                "Returns the approximate median (50th percentile) of input values. It is an alias of `approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY x)`.",
             )
             .with_syntax_example("approx_median(expression)")
             .with_sql_example(r#"```sql
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 0787506222f1c..b84a75770c6b5 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -317,14 +317,14 @@ fn get_approx_percentile_cont_doc() -> &'static Documentation {
             .with_description(
                 "Returns the approximate percentile of input values using the t-digest algorithm.",
             )
-            .with_syntax_example("approx_percentile_cont(expression, percentile, centroids)")
+            .with_syntax_example("approx_percentile_cont(percentile, centroids) WITHIN GROUP (ORDER BY expression)")
             .with_sql_example(r#"```sql
-> SELECT approx_percentile_cont(column_name, 0.75, 100) FROM table_name;
-+-------------------------------------------------+
-| approx_percentile_cont(column_name, 0.75, 100)  |
-+-------------------------------------------------+
-| 65.0                                            |
-+-------------------------------------------------+
+> SELECT approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) FROM table_name;
++-----------------------------------------------------------------------+
+| approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) |
++-----------------------------------------------------------------------+
+| 65.0                                                                  |
++-----------------------------------------------------------------------+
 ```"#)
             .with_standard_argument("expression", None)
             .with_argument("percentile", "Percentile to compute. Must be a float value between 0 and 1 (inclusive).")
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 4a82b6878084e..5506b1bef33b9 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
@@ -174,14 +174,14 @@ fn get_approx_percentile_cont_with_weight_doc() -> &'static Documentation {
             .with_description(
                 "Returns the weighted approximate percentile of input values using the t-digest algorithm.",
             )
-            .with_syntax_example("approx_percentile_cont_with_weight(expression, weight, percentile)")
+            .with_syntax_example("approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (ORDER BY expression)")
             .with_sql_example(r#"```sql
-> SELECT approx_percentile_cont_with_weight(column_name, weight_column, 0.90) FROM table_name;
-+----------------------------------------------------------------------+
-| approx_percentile_cont_with_weight(column_name, weight_column, 0.90) |
-+----------------------------------------------------------------------+
-| 78.5                                                                 |
-+----------------------------------------------------------------------+
+> SELECT approx_percentile_cont_with_weight(weight_column, 0.90) WITHIN GROUP (ORDER BY column_name) FROM table_name;
++---------------------------------------------------------------------------------------------+
+| approx_percentile_cont_with_weight(weight_column, 0.90) WITHIN GROUP (ORDER BY column_name) |
++---------------------------------------------------------------------------------------------+
+| 78.5                                                                                        |
++---------------------------------------------------------------------------------------------+
 ```"#,
             )
             .with_standard_argument("expression", None)
diff --git a/datafusion/proto/tests/cases/roundtrip_physical_plan.rs b/datafusion/proto/tests/cases/roundtrip_physical_plan.rs
index efa462aa7a855..bce84cf793e63 100644
--- a/datafusion/proto/tests/cases/roundtrip_physical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_physical_plan.rs
@@ -476,7 +476,7 @@ fn rountrip_aggregate_with_approx_pencentile_cont() -> Result<()> {
         vec![col("b", &schema)?, lit(0.5)],
     )
     .schema(Arc::clone(&schema))
-    .alias("APPROX_PERCENTILE_CONT(b, 0.5)")
+    .alias("APPROX_PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY b)")
     .build()
     .map(Arc::new)?];
 
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index f79014dd7ae39..72ddbcb74b3fa 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -176,11 +176,11 @@ impl FunctionArgs {
         }
 
         if !within_group.is_empty() && order_by.is_some() {
-            return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used.");
+            return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used");
         }
 
         if within_group.len() > 1 {
-            return not_impl_err!("Multiple column ordering in WITHIN GROUP clause is not supported: {within_group:?}");
+            return not_impl_err!("Multiple column ordering in WITHIN GROUP clause is not supported");
         }
 
         let order_by = order_by.unwrap_or_default();
@@ -334,13 +334,13 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
                 if let Some(fm) = self.context_provider.get_ordered_set_aggregate_meta(&name) {
                     if within_group.is_empty() {
-                        return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({}).", fm.name());
+                        return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
                     }
                 }
 
                 if null_treatment.is_some() &&
                     !fm.supports_null_handling_clause().unwrap_or(true) {
-                    return plan_err!("[IGNORE | RESPECT] NULLS are not permitted for {}.", fm.name());
+                    return plan_err!("[IGNORE | RESPECT] NULLS are not permitted for {}", fm.name());
                 }
 
                 let order_by = self.order_by_to_sort_expr(
diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt
index ecae13885b832..54c0642371d22 100644
--- a/datafusion/sqllogictest/test_files/aggregate.slt
+++ b/datafusion/sqllogictest/test_files/aggregate.slt
@@ -104,6 +104,18 @@ SELECT approx_percentile_cont(c12) WITHIN GROUP (ORDER BY c12) FROM aggregate_te
 statement error DataFusion error: This feature is not implemented: Tdigest max_size value for 'APPROX_PERCENTILE_CONT' must be a literal
 SELECT approx_percentile_cont(0.95, c5) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
 
+statement error DataFusion error: This feature is not implemented: Conflicting ordering requirements in aggregate functions is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5), approx_percentile_cont(0.2) WITHIN GROUP (ORDER BY c12) FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) IGNORE NULLS FROM aggregate_test_100
+
+statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) RESPECT NULLS FROM aggregate_test_100
+
+statement error DataFusion error: This feature is not implemented: Multiple column ordering in WITHIN GROUP clause is not supported
+SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5, c12) FROM aggregate_test_100
+
 # Not supported over sliding windows
 query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
 SELECT approx_percentile_cont(0.5)
diff --git a/docs/source/user-guide/sql/aggregate_functions.md b/docs/source/user-guide/sql/aggregate_functions.md
index d9fc28a81772d..aeaee6b1dc4d9 100644
--- a/docs/source/user-guide/sql/aggregate_functions.md
+++ b/docs/source/user-guide/sql/aggregate_functions.md
@@ -787,7 +787,7 @@ approx_distinct(expression)
 
 ### `approx_median`
 
-Returns the approximate median (50th percentile) of input values. It is an alias of `approx_percentile_cont(x, 0.5)`.
+Returns the approximate median (50th percentile) of input values. It is an alias of `approx_percentile_cont(0.5) WITHIN GROUP (ORDER BY x)`.
 
 ```
 approx_median(expression)
@@ -813,24 +813,24 @@ approx_median(expression)
 Returns the approximate percentile of input values using the t-digest algorithm.
 
 ```
-approx_percentile_cont(expression, percentile, centroids)
+approx_percentile_cont(percentile, centroids) WITHIN GROUP (expression [ORDER BY expression])
 ```
 
 #### Arguments
 
-- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 - **percentile**: Percentile to compute. Must be a float value between 0 and 1 (inclusive).
 - **centroids**: Number of centroids to use in the t-digest algorithm. _Default is 100_. A higher number results in more accurate approximation but requires more memory.
+- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 
 #### Example
 
 ```sql
-> SELECT approx_percentile_cont(column_name, 0.75, 100) FROM table_name;
-+-------------------------------------------------+
-| approx_percentile_cont(column_name, 0.75, 100)  |
-+-------------------------------------------------+
-| 65.0                                            |
-+-------------------------------------------------+
+> SELECT approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) FROM table_name;
++-----------------------------------------------------------------------+
+| approx_percentile_cont(0.75, 100) WITHIN GROUP (ORDER BY column_name) |
++-----------------------------------------------------------------------+
+| 65.0                                                                  |
++-----------------------------------------------------------------------+
 ```
 
 ### `approx_percentile_cont_with_weight`
@@ -838,22 +838,22 @@ approx_percentile_cont(expression, percentile, centroids)
 Returns the weighted approximate percentile of input values using the t-digest algorithm.
 
 ```
-approx_percentile_cont_with_weight(expression, weight, percentile)
+approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (expression [ORDER BY expression]) 
 ```
 
 #### Arguments
 
-- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 - **weight**: Expression to use as weight. Can be a constant, column, or function, and any combination of arithmetic operators.
 - **percentile**: Percentile to compute. Must be a float value between 0 and 1 (inclusive).
+- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 
 #### Example
 
 ```sql
-> SELECT approx_percentile_cont_with_weight(column_name, weight_column, 0.90) FROM table_name;
-+----------------------------------------------------------------------+
-| approx_percentile_cont_with_weight(column_name, weight_column, 0.90) |
-+----------------------------------------------------------------------+
-| 78.5                                                                 |
-+----------------------------------------------------------------------+
+> SELECT approx_percentile_cont_with_weight(weight_column, 0.90) WITHIN GROUP (ORDER BY column_name) FROM table_name;
++---------------------------------------------------------------------------------------------+
+| approx_percentile_cont_with_weight(weight_column, 0.90) WITHIN GROUP (ORDER BY column_name) |
++---------------------------------------------------------------------------------------------+
+| 78.5                                                                                        |
++---------------------------------------------------------------------------------------------+
 ```

From a827c9dac0a814ce288e94c4ef1baa9402538657 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 30 Jan 2025 21:16:17 +0900
Subject: [PATCH 10/39] Fix bug : handle missing within_group when applying
 children tree node

---
 datafusion/expr/src/tree_node.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs
index 5ab69d3da4c8d..9235597b9da1b 100644
--- a/datafusion/expr/src/tree_node.rs
+++ b/datafusion/expr/src/tree_node.rs
@@ -87,8 +87,13 @@ impl TreeNode for Expr {
                           }) => (expr, low, high).apply_ref_elements(f),
             Expr::Case(Case { expr, when_then_expr, else_expr }) =>
                 (expr, when_then_expr, else_expr).apply_ref_elements(f),
-            Expr::AggregateFunction(AggregateFunction { args, filter, order_by, .. }) =>
-                (args, filter, order_by).apply_ref_elements(f),
+            Expr::AggregateFunction(AggregateFunction { args, filter, order_by, within_group, .. }) => {
+                if within_group.is_some() {
+                    (args, filter, within_group).apply_ref_elements(f)
+                } else {
+                    (args, filter, order_by).apply_ref_elements(f)
+                }
+            }
             Expr::WindowFunction(WindowFunction {
                                      args,
                                      partition_by,

From 23bdf700f9e0052460293fbc9c5fbf2310d87bcb Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 30 Jan 2025 21:32:38 +0900
Subject: [PATCH 11/39] Change the signature of approx_percentile_cont for
 consistency

---
 datafusion/core/tests/dataframe/dataframe_functions.rs     | 6 +++---
 .../functions-aggregate/src/approx_percentile_cont.rs      | 7 ++++---
 datafusion/proto/tests/cases/roundtrip_logical_plan.rs     | 4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 8871443e16ebf..ebdcc99e10f34 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -360,7 +360,7 @@ async fn test_fn_approx_median() -> Result<()> {
 
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
-    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
+    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
 
     let expected = [
         "+----------------------------------------------------------------------------------+",
@@ -381,7 +381,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         None::<&str>,
         "arg_2".to_string(),
     ));
-    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
+    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
         "+---------------------------------------------------------------------------+",
@@ -395,7 +395,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_batches_eq!(expected, &batches);
 
     // with number of centroids set
-    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), Some(lit(2)));
+    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), Some(lit(2)));
     let expected = [
         "+-------------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index bb0aa0417b019..b4ad25842008e 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -53,12 +53,13 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
 
 /// Computes the approximate percentile continuous of a set of numbers
 pub fn approx_percentile_cont(
-    within_group: Vec<Sort>,
+    within_group: Option<Vec<Sort>>,
     percentile: Expr,
     centroids: Option<Expr>,
 ) -> Expr {
     let expr = within_group
-        .first()
+        .as_ref()
+        .and_then(|v| v.first())
         .map(|sort| sort.expr.clone())
         .unwrap_or_default();
 
@@ -75,7 +76,7 @@ pub fn approx_percentile_cont(
         None,
         None,
         None,
-        Some(within_group),
+        within_group,
     ))
 }
 
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index a13b1d82a607c..c6d0a4cb43b6c 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -970,8 +970,8 @@ async fn roundtrip_expr_api() -> Result<()> {
         stddev_pop(lit(2.2)),
         approx_distinct(lit(2)),
         approx_median(lit(2)),
-        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), None),
-        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), Some(lit(50))),
+        approx_percentile_cont(Some(vec![lit(2).sort(true, false)]), lit(0.5), None),
+        approx_percentile_cont(Some(vec![lit(2).sort(true, false)]), lit(0.5), Some(lit(50))),
         approx_percentile_cont_with_weight(lit(2), lit(1), lit(0.5)),
         grouping(lit(1)),
         bit_and(lit(2)),

From 97d96ca98c96938267011f7262576aa03034484f Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 30 Jan 2025 21:34:39 +0900
Subject: [PATCH 12/39] Add missing within_group for expr display

---
 datafusion/expr/src/expr.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index 97b3f75bc200b..e86d17eac80d6 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -2669,7 +2669,7 @@ impl Display for Expr {
                 filter,
                 order_by,
                 null_treatment,
-                ..
+                within_group,
             }) => {
                 fmt_function(f, func.name(), *distinct, args, true)?;
                 if let Some(nt) = null_treatment {
@@ -2681,6 +2681,9 @@ impl Display for Expr {
                 if let Some(ob) = order_by {
                     write!(f, " ORDER BY [{}]", expr_vec_fmt!(ob))?;
                 }
+                if let Some(wg) = within_group {
+                    write!(f, " WITHIN GROUP [{}]", expr_vec_fmt!(wg))?;
+                }
                 Ok(())
             }
             Expr::Between(Between {

From 1b61b5bb1fa984016059157ba97d189fd51271dc Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 31 Jan 2025 21:26:46 +0900
Subject: [PATCH 13/39] Handle edge case when over and within group clause are
 used together

---
 datafusion/sql/src/expr/function.rs              | 5 +++++
 datafusion/sqllogictest/test_files/aggregate.slt | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 4ff73f4ac8be2..fbd67b9943811 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -222,6 +222,11 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
             within_group,
         } = function_args;
 
+        if over.is_some() && !within_group.is_empty() {
+            return plan_err!("OVER and WITHIN GROUP clause are can not be used together. \
+                OVER is for window function, whereas WITHIN GROUP is for ordered set aggregate function");
+        }
+
         // If function is a window function (it has an OVER clause),
         // it shouldn't have ordering requirement as function argument
         // required ordering should be defined in OVER clause.
diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt
index f1279012aada0..5bd6d05a0b67c 100644
--- a/datafusion/sqllogictest/test_files/aggregate.slt
+++ b/datafusion/sqllogictest/test_files/aggregate.slt
@@ -117,7 +117,7 @@ statement error DataFusion error: This feature is not implemented: Multiple colu
 SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5, c12) FROM aggregate_test_100
 
 # Not supported over sliding windows
-query error This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented
+query error DataFusion error: Error during planning: OVER and WITHIN GROUP clause are can not be used together. OVER is for window function, whereas WITHIN GROUP is for ordered set aggregate function
 SELECT approx_percentile_cont(0.5)
 WITHIN GROUP (ORDER BY c3)
 OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)

From d0fdde3ea64eb1e1bd04a31fb41264dec690b436 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Feb 2025 13:27:20 +0900
Subject: [PATCH 14/39] Apply clippy advice: avoids too many arguments

---
 datafusion/catalog/src/session.rs             | 12 +++--
 .../core/src/execution/session_state.rs       | 12 +++--
 datafusion/execution/src/lib.rs               |  2 +-
 datafusion/execution/src/task.rs              | 46 ++++++++++++++-----
 4 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/datafusion/catalog/src/session.rs b/datafusion/catalog/src/session.rs
index 373866b61a4fe..4ef458aab7338 100644
--- a/datafusion/catalog/src/session.rs
+++ b/datafusion/catalog/src/session.rs
@@ -20,7 +20,7 @@ use datafusion_common::config::ConfigOptions;
 use datafusion_common::{DFSchema, Result};
 use datafusion_execution::config::SessionConfig;
 use datafusion_execution::runtime_env::RuntimeEnv;
-use datafusion_execution::TaskContext;
+use datafusion_execution::{TaskContext, TaskContextFunctionParams};
 use datafusion_expr::execution_props::ExecutionProps;
 use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
 use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
@@ -135,10 +135,12 @@ impl From<&dyn Session> for TaskContext {
             task_id,
             state.session_id().to_string(),
             state.config().clone(),
-            state.scalar_functions().clone(),
-            state.aggregate_functions().clone(),
-            state.ordered_set_aggregate_functions().clone(),
-            state.window_functions().clone(),
+            TaskContextFunctionParams::new(
+                state.scalar_functions().clone(),
+                state.aggregate_functions().clone(),
+                state.ordered_set_aggregate_functions().clone(),
+                state.window_functions().clone(),
+            ),
             state.runtime_env().clone(),
         )
     }
diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs
index 78386f892481e..d1ae4f318c5c6 100644
--- a/datafusion/core/src/execution/session_state.rs
+++ b/datafusion/core/src/execution/session_state.rs
@@ -48,7 +48,7 @@ use datafusion_common::{
 };
 use datafusion_execution::config::SessionConfig;
 use datafusion_execution::runtime_env::RuntimeEnv;
-use datafusion_execution::TaskContext;
+use datafusion_execution::{TaskContext, TaskContextFunctionParams};
 use datafusion_expr::execution_props::ExecutionProps;
 use datafusion_expr::expr_rewriter::FunctionRewrite;
 use datafusion_expr::planner::{ExprPlanner, TypePlanner};
@@ -2013,10 +2013,12 @@ impl From<&SessionState> for TaskContext {
             task_id,
             state.session_id.clone(),
             state.config.clone(),
-            state.scalar_functions.clone(),
-            state.aggregate_functions.clone(),
-            state.ordered_set_aggregate_functions.clone(),
-            state.window_functions.clone(),
+            TaskContextFunctionParams::new(
+                state.scalar_functions.clone(),
+                state.aggregate_functions.clone(),
+                state.ordered_set_aggregate_functions.clone(),
+                state.window_functions.clone(),
+            ),
             Arc::clone(&state.runtime_env),
         )
     }
diff --git a/datafusion/execution/src/lib.rs b/datafusion/execution/src/lib.rs
index 317bd3203ab1b..6340d774535ed 100644
--- a/datafusion/execution/src/lib.rs
+++ b/datafusion/execution/src/lib.rs
@@ -38,4 +38,4 @@ pub mod registry {
 pub use disk_manager::DiskManager;
 pub use registry::FunctionRegistry;
 pub use stream::{RecordBatchStream, SendableRecordBatchStream};
-pub use task::TaskContext;
+pub use task::{TaskContext, TaskContextFunctionParams};
diff --git a/datafusion/execution/src/task.rs b/datafusion/execution/src/task.rs
index cd6fca51bf356..9984f90b5b118 100644
--- a/datafusion/execution/src/task.rs
+++ b/datafusion/execution/src/task.rs
@@ -82,20 +82,17 @@ impl TaskContext {
         task_id: Option<String>,
         session_id: String,
         session_config: SessionConfig,
-        scalar_functions: HashMap<String, Arc<ScalarUDF>>,
-        aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-        ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-        window_functions: HashMap<String, Arc<WindowUDF>>,
+        function_params: TaskContextFunctionParams,
         runtime: Arc<RuntimeEnv>,
     ) -> Self {
         Self {
             task_id,
             session_id,
             session_config,
-            scalar_functions,
-            aggregate_functions,
-            ordered_set_aggregate_functions,
-            window_functions,
+            scalar_functions: function_params.scalar_functions,
+            aggregate_functions: function_params.aggregate_functions,
+            ordered_set_aggregate_functions: function_params.ordered_set_aggregate_functions,
+            window_functions: function_params.window_functions,
             runtime,
         }
     }
@@ -154,6 +151,29 @@ impl TaskContext {
     }
 }
 
+pub struct TaskContextFunctionParams {
+    scalar_functions: HashMap<String, Arc<ScalarUDF>>,
+    aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+    window_functions: HashMap<String, Arc<WindowUDF>>,
+}
+
+impl TaskContextFunctionParams {
+    pub fn new(
+        scalar_functions: HashMap<String, Arc<ScalarUDF>>,
+        aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+        ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+        window_functions: HashMap<String, Arc<WindowUDF>>,
+    ) -> Self {
+        Self {
+            scalar_functions,
+            aggregate_functions,
+            ordered_set_aggregate_functions,
+            window_functions,
+        }
+    }
+}
+
 impl FunctionRegistry for TaskContext {
     fn udfs(&self) -> HashSet<String> {
         self.scalar_functions.keys().cloned().collect()
@@ -266,10 +286,12 @@ mod tests {
             Some("task_id".to_string()),
             "session_id".to_string(),
             session_config,
-            HashMap::default(),
-            HashMap::default(),
-            HashMap::default(),
-            HashMap::default(),
+            TaskContextFunctionParams::new(
+                HashMap::default(),
+                HashMap::default(),
+                HashMap::default(),
+                HashMap::default(),
+            ),
             runtime,
         );
 

From 3c8bce31a967af3bfa7e2b2e4a7a13912f89b570 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Feb 2025 13:28:05 +0900
Subject: [PATCH 15/39] Add new test cases using descending order

---
 .../sqllogictest/test_files/aggregate.slt      | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt
index 5bd6d05a0b67c..b1099996809cf 100644
--- a/datafusion/sqllogictest/test_files/aggregate.slt
+++ b/datafusion/sqllogictest/test_files/aggregate.slt
@@ -1645,6 +1645,15 @@ c 122
 d 124
 e 115
 
+query TI
+SELECT c1, approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c3 DESC) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+----
+a -101
+b -114
+c -109
+d -98
+e -93
+
 # csv_query_approx_percentile_cont_with_weight (2)
 query TI
 SELECT c1, approx_percentile_cont_with_weight(1, 0.95) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
@@ -1655,6 +1664,15 @@ c 122
 d 124
 e 115
 
+query TI
+SELECT c1, approx_percentile_cont_with_weight(1, 0.95) WITHIN GROUP (ORDER BY c3 DESC) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
+----
+a -101
+b -114
+c -109
+d -98
+e -93
+
 # csv_query_approx_percentile_cont_with_histogram_bins
 query TI
 SELECT c1, approx_percentile_cont(0.95, 200) WITHIN GROUP (ORDER BY c3) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1

From be99a3551982ab347ff279add5d5fadfd9ddc190 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Feb 2025 13:30:41 +0900
Subject: [PATCH 16/39] Apply cargo fmt

---
 .../core/src/execution/session_state.rs       | 26 ++++++++++++++-----
 .../tests/dataframe/dataframe_functions.rs    | 12 ++++++---
 datafusion/execution/src/task.rs              |  7 +++--
 datafusion/expr/src/expr.rs                   | 25 +++++++++++-------
 datafusion/expr/src/registry.rs               | 10 +++----
 datafusion/expr/src/tree_node.rs              |  2 +-
 .../tests/cases/roundtrip_logical_plan.rs     |  6 ++++-
 datafusion/sql/src/expr/function.rs           | 21 ++++++++++-----
 datafusion/sql/src/expr/mod.rs                |  5 +++-
 datafusion/sql/src/unparser/expr.rs           |  3 ++-
 datafusion/sql/tests/common/mod.rs            |  5 +++-
 .../substrait/src/logical_plan/consumer.rs    | 19 ++++++++++----
 .../substrait/src/logical_plan/producer.rs    | 20 ++++++--------
 13 files changed, 107 insertions(+), 54 deletions(-)

diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs
index d1ae4f318c5c6..647bec15246e2 100644
--- a/datafusion/core/src/execution/session_state.rs
+++ b/datafusion/core/src/execution/session_state.rs
@@ -208,7 +208,10 @@ impl Debug for SessionState {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
-            .field("ordered_set_aggregate_functions", &self.ordered_set_aggregate_functions)
+            .field(
+                "ordered_set_aggregate_functions",
+                &self.ordered_set_aggregate_functions,
+            )
             .field("window_functions", &self.window_functions)
             .field("prepared_plans", &self.prepared_plans)
             .finish()
@@ -1080,7 +1083,10 @@ impl SessionStateBuilder {
                 existing.aggregate_functions.into_values().collect_vec(),
             ),
             ordered_set_aggregate_functions: Some(
-                existing.ordered_set_aggregate_functions.into_values().collect_vec(),
+                existing
+                    .ordered_set_aggregate_functions
+                    .into_values()
+                    .collect_vec(),
             ),
             window_functions: Some(existing.window_functions.into_values().collect_vec()),
             serializer_registry: Some(existing.serializer_registry),
@@ -1107,7 +1113,9 @@ impl SessionStateBuilder {
             .with_expr_planners(SessionStateDefaults::default_expr_planners())
             .with_scalar_functions(SessionStateDefaults::default_scalar_functions())
             .with_aggregate_functions(SessionStateDefaults::default_aggregate_functions())
-            .with_ordered_set_aggregate_functions(SessionStateDefaults::default_ordered_set_aggregate_functions())
+            .with_ordered_set_aggregate_functions(
+                SessionStateDefaults::default_ordered_set_aggregate_functions(),
+            )
             .with_window_functions(SessionStateDefaults::default_window_functions())
             .with_table_function_list(SessionStateDefaults::default_table_functions())
     }
@@ -1686,7 +1694,10 @@ impl Debug for SessionStateBuilder {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
-            .field("ordered_set_aggregate_functions", &self.ordered_set_aggregate_functions)
+            .field(
+                "ordered_set_aggregate_functions",
+                &self.ordered_set_aggregate_functions,
+            )
             .field("window_functions", &self.window_functions)
             .finish()
     }
@@ -1857,7 +1868,9 @@ impl FunctionRegistry for SessionState {
         let result = self.ordered_set_aggregate_functions.get(name);
 
         result.cloned().ok_or_else(|| {
-            plan_datafusion_err!("There is no ordered set UDAF named \"{name}\" in the registry")
+            plan_datafusion_err!(
+                "There is no ordered set UDAF named \"{name}\" in the registry"
+            )
         })
     }
 
@@ -1899,7 +1912,8 @@ impl FunctionRegistry for SessionState {
             self.ordered_set_aggregate_functions
                 .insert(alias.clone(), Arc::clone(&ordered_set_udaf));
         });
-        Ok(self.ordered_set_aggregate_functions
+        Ok(self
+            .ordered_set_aggregate_functions
             .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
     }
 
diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index ebdcc99e10f34..4cf5b7e402b88 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -360,7 +360,8 @@ async fn test_fn_approx_median() -> Result<()> {
 
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
-    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
+    let expr =
+        approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
 
     let expected = [
         "+----------------------------------------------------------------------------------+",
@@ -381,7 +382,8 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         None::<&str>,
         "arg_2".to_string(),
     ));
-    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), alias_expr, None);
+    let expr =
+        approx_percentile_cont(Some(vec![col("b").sort(true, false)]), alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
         "+---------------------------------------------------------------------------+",
@@ -395,7 +397,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_batches_eq!(expected, &batches);
 
     // with number of centroids set
-    let expr = approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), Some(lit(2)));
+    let expr = approx_percentile_cont(
+        Some(vec![col("b").sort(true, false)]),
+        lit(0.5),
+        Some(lit(2)),
+    );
     let expected = [
         "+-------------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
diff --git a/datafusion/execution/src/task.rs b/datafusion/execution/src/task.rs
index 9984f90b5b118..d031c52127a28 100644
--- a/datafusion/execution/src/task.rs
+++ b/datafusion/execution/src/task.rs
@@ -91,7 +91,8 @@ impl TaskContext {
             session_config,
             scalar_functions: function_params.scalar_functions,
             aggregate_functions: function_params.aggregate_functions,
-            ordered_set_aggregate_functions: function_params.ordered_set_aggregate_functions,
+            ordered_set_aggregate_functions: function_params
+                .ordered_set_aggregate_functions,
             window_functions: function_params.window_functions,
             runtime,
         }
@@ -199,7 +200,9 @@ impl FunctionRegistry for TaskContext {
         let result = self.ordered_set_aggregate_functions.get(name);
 
         result.cloned().ok_or_else(|| {
-            plan_datafusion_err!("There is no ordered set UDAF named \"{name}\" in the TaskContext")
+            plan_datafusion_err!(
+                "There is no ordered set UDAF named \"{name}\" in the TaskContext"
+            )
         })
     }
 
diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index e86d17eac80d6..9f4836072e230 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -1903,17 +1903,18 @@ impl NormalizeEq for Expr {
                         _ => false,
                     }
                     && match (self_within_group, other_within_group) {
-                        (Some(self_within_group), Some(other_within_group)) => self_within_group
-                            .iter()
-                            .zip(other_within_group.iter())
-                            .all(|(a, b)| {
-                                a.asc == b.asc
-                                    && a.nulls_first == b.nulls_first
-                                    && a.expr.normalize_eq(&b.expr)
-                            }),
+                        (Some(self_within_group), Some(other_within_group)) => {
+                            self_within_group.iter().zip(other_within_group.iter()).all(
+                                |(a, b)| {
+                                    a.asc == b.asc
+                                        && a.nulls_first == b.nulls_first
+                                        && a.expr.normalize_eq(&b.expr)
+                                },
+                            )
+                        }
                         (None, None) => true,
                         _ => false,
-                }
+                    }
             }
             (
                 Expr::WindowFunction(WindowFunction {
@@ -2303,7 +2304,11 @@ impl Display for SchemaDisplay<'_> {
                 };
 
                 if let Some(within_group) = within_group {
-                    write!(f, " WITHIN GROUP [{}]", schema_name_from_sorts(within_group)?)?;
+                    write!(
+                        f,
+                        " WITHIN GROUP [{}]",
+                        schema_name_from_sorts(within_group)?
+                    )?;
                 };
 
                 Ok(())
diff --git a/datafusion/expr/src/registry.rs b/datafusion/expr/src/registry.rs
index 4f43289300a7d..609aa096dde19 100644
--- a/datafusion/expr/src/registry.rs
+++ b/datafusion/expr/src/registry.rs
@@ -217,10 +217,9 @@ impl FunctionRegistry for MemoryFunctionRegistry {
     }
 
     fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-        self.ordered_set_udafs
-            .get(name)
-            .cloned()
-            .ok_or_else(|| plan_datafusion_err!("Ordered Set Aggregate Function {name} not found"))
+        self.ordered_set_udafs.get(name).cloned().ok_or_else(|| {
+            plan_datafusion_err!("Ordered Set Aggregate Function {name} not found")
+        })
     }
 
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
@@ -243,7 +242,8 @@ impl FunctionRegistry for MemoryFunctionRegistry {
         &mut self,
         ordered_set_udaf: Arc<AggregateUDF>,
     ) -> Result<Option<Arc<AggregateUDF>>> {
-        Ok(self.ordered_set_udafs
+        Ok(self
+            .ordered_set_udafs
             .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
     }
     fn register_udwf(&mut self, udaf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs
index 9235597b9da1b..2602554232aac 100644
--- a/datafusion/expr/src/tree_node.rs
+++ b/datafusion/expr/src/tree_node.rs
@@ -283,7 +283,7 @@ impl TreeNode for Expr {
                         },
                     )?
                 }
-            },
+            }
             Expr::GroupingSet(grouping_set) => match grouping_set {
                 GroupingSet::Rollup(exprs) => exprs
                     .map_elements(f)?
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index c6d0a4cb43b6c..dd83f384b9dff 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -971,7 +971,11 @@ async fn roundtrip_expr_api() -> Result<()> {
         approx_distinct(lit(2)),
         approx_median(lit(2)),
         approx_percentile_cont(Some(vec![lit(2).sort(true, false)]), lit(0.5), None),
-        approx_percentile_cont(Some(vec![lit(2).sort(true, false)]), lit(0.5), Some(lit(50))),
+        approx_percentile_cont(
+            Some(vec![lit(2).sort(true, false)]),
+            lit(0.5),
+            Some(lit(50)),
+        ),
         approx_percentile_cont_with_weight(lit(2), lit(1), lit(0.5)),
         grouping(lit(1)),
         bit_and(lit(2)),
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index fbd67b9943811..f75e811ff777f 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -185,7 +185,9 @@ impl FunctionArgs {
         }
 
         if within_group.len() > 1 {
-            return not_impl_err!("Multiple column ordering in WITHIN GROUP clause is not supported");
+            return not_impl_err!(
+                "Multiple column ordering in WITHIN GROUP clause is not supported"
+            );
         }
 
         let order_by = order_by.unwrap_or_default();
@@ -342,15 +344,21 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
-                if let Some(fm) = self.context_provider.get_ordered_set_aggregate_meta(&name) {
+                if let Some(fm) =
+                    self.context_provider.get_ordered_set_aggregate_meta(&name)
+                {
                     if within_group.is_empty() {
                         return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
                     }
                 }
 
-                if null_treatment.is_some() &&
-                    !fm.supports_null_handling_clause().unwrap_or(true) {
-                    return plan_err!("[IGNORE | RESPECT] NULLS are not permitted for {}", fm.name());
+                if null_treatment.is_some()
+                    && !fm.supports_null_handling_clause().unwrap_or(true)
+                {
+                    return plan_err!(
+                        "[IGNORE | RESPECT] NULLS are not permitted for {}",
+                        fm.name()
+                    );
                 }
 
                 let order_by = self.order_by_to_sort_expr(
@@ -361,7 +369,8 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                     None,
                 )?;
                 let order_by = (!order_by.is_empty()).then_some(order_by);
-                let mut args = self.function_args_to_expr(args, schema, planner_context)?;
+                let mut args =
+                    self.function_args_to_expr(args, schema, planner_context)?;
 
                 let within_group = self.order_by_to_sort_expr(
                     within_group,
diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs
index 87d585710342c..f809162e686e5 100644
--- a/datafusion/sql/src/expr/mod.rs
+++ b/datafusion/sql/src/expr/mod.rs
@@ -1125,7 +1125,10 @@ mod tests {
             }
         }
 
-        fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
+        fn get_ordered_set_aggregate_meta(
+            &self,
+            _name: &str,
+        ) -> Option<Arc<AggregateUDF>> {
             None
         }
 
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index 3d941c06e8b82..bcd818030a720 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -279,7 +279,8 @@ impl Unparser<'_> {
                     None => None,
                 };
 
-                let within_group = agg.within_group
+                let within_group = agg
+                    .within_group
                     .as_ref()
                     .unwrap_or(&Vec::new())
                     .iter()
diff --git a/datafusion/sql/tests/common/mod.rs b/datafusion/sql/tests/common/mod.rs
index d1628f15fd366..5dbc106f97ee7 100644
--- a/datafusion/sql/tests/common/mod.rs
+++ b/datafusion/sql/tests/common/mod.rs
@@ -238,7 +238,10 @@ impl ContextProvider for MockContextProvider {
     }
 
     fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
-        self.state.ordered_set_aggregate_functions.get(name).cloned()
+        self.state
+            .ordered_set_aggregate_functions
+            .get(name)
+            .cloned()
     }
 
     fn get_variable_type(&self, _: &[String]) -> Option<DataType> {
diff --git a/datafusion/substrait/src/logical_plan/consumer.rs b/datafusion/substrait/src/logical_plan/consumer.rs
index 18557924bcc7b..70fbbdffa9a0a 100644
--- a/datafusion/substrait/src/logical_plan/consumer.rs
+++ b/datafusion/substrait/src/logical_plan/consumer.rs
@@ -1952,13 +1952,22 @@ pub async fn from_substrait_agg_func(
         args
     };
 
-    let (within_group, order_by) = match consumer.get_function_registry().ordered_set_udaf(fn_name) {
-        Ok(_) => (sorts, None),
-        Err(_) => (None, sorts),
-    };
+    let (within_group, order_by) =
+        match consumer.get_function_registry().ordered_set_udaf(fn_name) {
+            Ok(_) => (sorts, None),
+            Err(_) => (None, sorts),
+        };
 
     Ok(Arc::new(Expr::AggregateFunction(
-        expr::AggregateFunction::new_udf(udaf, args, distinct, filter, order_by, None, within_group),
+        expr::AggregateFunction::new_udf(
+            udaf,
+            args,
+            distinct,
+            filter,
+            order_by,
+            None,
+            within_group,
+        ),
     )))
 }
 
diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs
index fc113492cc2b8..2f0d5dddac63e 100644
--- a/datafusion/substrait/src/logical_plan/producer.rs
+++ b/datafusion/substrait/src/logical_plan/producer.rs
@@ -1217,18 +1217,14 @@ pub fn from_aggregate_function(
     } = agg_fn;
 
     let sorts = match (within_group, order_by) {
-        (Some(within_group), _) => {
-            within_group
-                .iter()
-                .map(|expr| to_substrait_sort_field(producer, expr, schema))
-                .collect::<Result<Vec<_>>>()?
-        }
-        (None, Some(order_by)) => {
-            order_by
-                .iter()
-                .map(|expr| to_substrait_sort_field(producer, expr, schema))
-                .collect::<Result<Vec<_>>>()?
-        }
+        (Some(within_group), _) => within_group
+            .iter()
+            .map(|expr| to_substrait_sort_field(producer, expr, schema))
+            .collect::<Result<Vec<_>>>()?,
+        (None, Some(order_by)) => order_by
+            .iter()
+            .map(|expr| to_substrait_sort_field(producer, expr, schema))
+            .collect::<Result<Vec<_>>>()?,
         (None, None) => vec![],
     };
 

From f9aa1fcacaad627848f024758279fa0fa4628b78 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Wed, 5 Feb 2025 10:10:23 +0900
Subject: [PATCH 17/39] Revert unintended submodule changes

---
 parquet-testing | 2 +-
 testing         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/parquet-testing b/parquet-testing
index e45cd23f784aa..f4d7ed772a62a 160000
--- a/parquet-testing
+++ b/parquet-testing
@@ -1 +1 @@
-Subproject commit e45cd23f784aab3d6bf0701f8f4e621469ed3be7
+Subproject commit f4d7ed772a62a95111db50fbcad2460833e8c882
diff --git a/testing b/testing
index 98fceecd024dc..d2a1371230349 160000
--- a/testing
+++ b/testing
@@ -1 +1 @@
-Subproject commit 98fceecd024dccd2f8a00e32fc144975f218acf4
+Subproject commit d2a13712303498963395318a4eb42872e66aead7

From d5f0b6260c348b3afacd5d1a91f09912235bd192 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Wed, 5 Feb 2025 10:11:07 +0900
Subject: [PATCH 18/39] Apply prettier guidance

---
 docs/source/user-guide/sql/aggregate_functions.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/source/user-guide/sql/aggregate_functions.md b/docs/source/user-guide/sql/aggregate_functions.md
index c3b82f171d79e..584bc8150bea6 100644
--- a/docs/source/user-guide/sql/aggregate_functions.md
+++ b/docs/source/user-guide/sql/aggregate_functions.md
@@ -838,7 +838,7 @@ approx_percentile_cont(percentile, centroids) WITHIN GROUP (expression [ORDER BY
 Returns the weighted approximate percentile of input values using the t-digest algorithm.
 
 ```
-approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (expression [ORDER BY expression]) 
+approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (expression [ORDER BY expression])
 ```
 
 #### Arguments

From d7f2f596271b915c38814ca788fec1c095c6f47f Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 7 Feb 2025 13:04:44 +0900
Subject: [PATCH 19/39] Apply doc guidance by update_function_doc.sh

---
 docs/source/user-guide/sql/aggregate_functions.md | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/source/user-guide/sql/aggregate_functions.md b/docs/source/user-guide/sql/aggregate_functions.md
index 584bc8150bea6..27ebbf3644243 100644
--- a/docs/source/user-guide/sql/aggregate_functions.md
+++ b/docs/source/user-guide/sql/aggregate_functions.md
@@ -813,14 +813,14 @@ approx_median(expression)
 Returns the approximate percentile of input values using the t-digest algorithm.
 
 ```
-approx_percentile_cont(percentile, centroids) WITHIN GROUP (expression [ORDER BY expression])
+approx_percentile_cont(percentile, centroids) WITHIN GROUP (ORDER BY expression)
 ```
 
 #### Arguments
 
+- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 - **percentile**: Percentile to compute. Must be a float value between 0 and 1 (inclusive).
 - **centroids**: Number of centroids to use in the t-digest algorithm. _Default is 100_. A higher number results in more accurate approximation but requires more memory.
-- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 
 #### Example
 
@@ -838,14 +838,14 @@ approx_percentile_cont(percentile, centroids) WITHIN GROUP (expression [ORDER BY
 Returns the weighted approximate percentile of input values using the t-digest algorithm.
 
 ```
-approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (expression [ORDER BY expression])
+approx_percentile_cont_with_weight(weight, percentile) WITHIN GROUP (ORDER BY expression)
 ```
 
 #### Arguments
 
+- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 - **weight**: Expression to use as weight. Can be a constant, column, or function, and any combination of arithmetic operators.
 - **percentile**: Percentile to compute. Must be a float value between 0 and 1 (inclusive).
-- **expression**: The expression to operate on. Can be a constant, column, or function, and any combination of operators.
 
 #### Example
 

From 91565b3d872db4ae14b95a5d661e0c14a06a5b23 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 27 Feb 2025 17:11:20 +0900
Subject: [PATCH 20/39] Rollback WITHIN GROUP and related logic after
 converting it into expr

* Make it not to handle redundant logic
---
 datafusion-examples/examples/advanced_udaf.rs |  1 -
 datafusion/core/src/physical_planner.rs       | 16 ++----
 .../core/tests/execution/logical_plan.rs      |  1 -
 datafusion/expr/src/expr.rs                   | 37 -------------
 datafusion/expr/src/expr_fn.rs                | 24 ---------
 datafusion/expr/src/test/function_stub.rs     |  5 --
 datafusion/expr/src/tree_node.rs              | 53 +++++--------------
 datafusion/expr/src/udaf.rs                   |  1 -
 .../src/approx_percentile_cont.rs             |  1 -
 datafusion/functions-aggregate/src/count.rs   |  2 +-
 datafusion/functions-aggregate/src/macros.rs  |  2 -
 datafusion/functions-nested/src/planner.rs    |  1 -
 .../optimizer/src/analyzer/type_coercion.rs   |  7 ---
 .../optimizer/src/common_subexpr_eliminate.rs |  1 -
 .../simplify_expressions/expr_simplifier.rs   |  2 -
 .../src/single_distinct_to_groupby.rs         |  8 +--
 .../proto/src/logical_plan/from_proto.rs      |  4 --
 datafusion/proto/src/logical_plan/to_proto.rs |  5 --
 .../tests/cases/roundtrip_logical_plan.rs     |  1 -
 datafusion/sql/src/expr/function.rs           |  1 -
 .../substrait/src/logical_plan/consumer.rs    | 13 ++---
 .../substrait/src/logical_plan/producer.rs    | 16 ++----
 22 files changed, 28 insertions(+), 174 deletions(-)

diff --git a/datafusion-examples/examples/advanced_udaf.rs b/datafusion-examples/examples/advanced_udaf.rs
index 8b47f3dffcbbb..fd65c3352bbc8 100644
--- a/datafusion-examples/examples/advanced_udaf.rs
+++ b/datafusion-examples/examples/advanced_udaf.rs
@@ -428,7 +428,6 @@ impl AggregateUDFImpl for SimplifiedGeoMeanUdaf {
                 aggregate_function.filter,
                 aggregate_function.order_by,
                 aggregate_function.null_treatment,
-                aggregate_function.within_group,
             )))
         };
         Some(Box::new(simplify))
diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs
index e19d74b1fb543..cee3acc08daec 100644
--- a/datafusion/core/src/physical_planner.rs
+++ b/datafusion/core/src/physical_planner.rs
@@ -1597,7 +1597,6 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
             filter,
             order_by,
             null_treatment,
-            within_group,
         }) => {
             let name = if let Some(name) = name {
                 name
@@ -1620,20 +1619,13 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
                 == NullTreatment::IgnoreNulls;
 
             let (agg_expr, filter, order_by) = {
-                let physical_sort_exprs = match within_group {
-                    Some(within_group) => Some(create_physical_sort_exprs(
-                        within_group,
+                let physical_sort_exprs = match order_by {
+                    Some(exprs) => Some(create_physical_sort_exprs(
+                        exprs,
                         logical_input_schema,
                         execution_props,
                     )?),
-                    None => match order_by {
-                        Some(order_by) => Some(create_physical_sort_exprs(
-                            order_by,
-                            logical_input_schema,
-                            execution_props,
-                        )?),
-                        None => None,
-                    },
+                    None => None,
                 };
 
                 let ordering_reqs: LexOrdering =
diff --git a/datafusion/core/tests/execution/logical_plan.rs b/datafusion/core/tests/execution/logical_plan.rs
index 9739bf9942e4a..9f093ccf24b60 100644
--- a/datafusion/core/tests/execution/logical_plan.rs
+++ b/datafusion/core/tests/execution/logical_plan.rs
@@ -65,7 +65,6 @@ async fn count_only_nulls() -> Result<()> {
             filter: None,
             order_by: None,
             null_treatment: None,
-            within_group: None,
         })],
     )?);
 
diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index c5b8bac83df9f..fd5186f185458 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -705,8 +705,6 @@ pub struct AggregateFunction {
     /// Optional ordering
     pub order_by: Option<Vec<Sort>>,
     pub null_treatment: Option<NullTreatment>,
-    /// Optional WITHIN GROUP for ordered-set aggregate functions
-    pub within_group: Option<Vec<Sort>>,
 }
 
 impl AggregateFunction {
@@ -718,7 +716,6 @@ impl AggregateFunction {
         filter: Option<Box<Expr>>,
         order_by: Option<Vec<Sort>>,
         null_treatment: Option<NullTreatment>,
-        within_group: Option<Vec<Sort>>,
     ) -> Self {
         Self {
             func,
@@ -727,7 +724,6 @@ impl AggregateFunction {
             filter,
             order_by,
             null_treatment,
-            within_group,
         }
     }
 }
@@ -873,8 +869,6 @@ pub struct AggregateUDF {
     pub filter: Option<Box<Expr>>,
     /// Optional ORDER BY applied prior to aggregating
     pub order_by: Option<Vec<Expr>>,
-    /// Optional WITHIN GROUP for ordered-set aggregate functions
-    pub within_group: Option<Vec<Expr>>,
 }
 
 impl AggregateUDF {
@@ -884,14 +878,12 @@ impl AggregateUDF {
         args: Vec<Expr>,
         filter: Option<Box<Expr>>,
         order_by: Option<Vec<Expr>>,
-        within_group: Option<Vec<Expr>>,
     ) -> Self {
         Self {
             fun,
             args,
             filter,
             order_by,
-            within_group,
         }
     }
 }
@@ -1877,7 +1869,6 @@ impl NormalizeEq for Expr {
                     filter: self_filter,
                     order_by: self_order_by,
                     null_treatment: self_null_treatment,
-                    within_group: self_within_group,
                 }),
                 Expr::AggregateFunction(AggregateFunction {
                     func: other_func,
@@ -1886,7 +1877,6 @@ impl NormalizeEq for Expr {
                     filter: other_filter,
                     order_by: other_order_by,
                     null_treatment: other_null_treatment,
-                    within_group: other_within_group,
                 }),
             ) => {
                 self_func.name() == other_func.name()
@@ -1916,19 +1906,6 @@ impl NormalizeEq for Expr {
                         (None, None) => true,
                         _ => false,
                     }
-                    && match (self_within_group, other_within_group) {
-                        (Some(self_within_group), Some(other_within_group)) => {
-                            self_within_group.iter().zip(other_within_group.iter()).all(
-                                |(a, b)| {
-                                    a.asc == b.asc
-                                        && a.nulls_first == b.nulls_first
-                                        && a.expr.normalize_eq(&b.expr)
-                                },
-                            )
-                        }
-                        (None, None) => true,
-                        _ => false,
-                    }
             }
             (
                 Expr::WindowFunction(WindowFunction {
@@ -2182,7 +2159,6 @@ impl HashNode for Expr {
                 filter: _filter,
                 order_by: _order_by,
                 null_treatment,
-                within_group: _within_group,
             }) => {
                 func.hash(state);
                 distinct.hash(state);
@@ -2295,7 +2271,6 @@ impl Display for SchemaDisplay<'_> {
                 filter,
                 order_by,
                 null_treatment,
-                within_group,
             }) => {
                 write!(
                     f,
@@ -2317,14 +2292,6 @@ impl Display for SchemaDisplay<'_> {
                     write!(f, " ORDER BY [{}]", schema_name_from_sorts(order_by)?)?;
                 };
 
-                if let Some(within_group) = within_group {
-                    write!(
-                        f,
-                        " WITHIN GROUP [{}]",
-                        schema_name_from_sorts(within_group)?
-                    )?;
-                };
-
                 Ok(())
             }
             // Expr is not shown since it is aliased
@@ -2693,7 +2660,6 @@ impl Display for Expr {
                 filter,
                 order_by,
                 null_treatment,
-                within_group,
             }) => {
                 fmt_function(f, func.name(), *distinct, args, true)?;
                 if let Some(nt) = null_treatment {
@@ -2705,9 +2671,6 @@ impl Display for Expr {
                 if let Some(ob) = order_by {
                     write!(f, " ORDER BY [{}]", expr_vec_fmt!(ob))?;
                 }
-                if let Some(wg) = within_group {
-                    write!(f, " WITHIN GROUP [{}]", expr_vec_fmt!(wg))?;
-                }
                 Ok(())
             }
             Expr::Between(Between {
diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs
index 54d8b722640b5..a2de5e7b259f1 100644
--- a/datafusion/expr/src/expr_fn.rs
+++ b/datafusion/expr/src/expr_fn.rs
@@ -769,8 +769,6 @@ pub trait ExprFunctionExt {
     fn partition_by(self, partition_by: Vec<Expr>) -> ExprFuncBuilder;
     /// Add appropriate window frame conditions
     fn window_frame(self, window_frame: WindowFrame) -> ExprFuncBuilder;
-    /// Add `WITHIN GROUP <within_group>`
-    fn within_group(self, within_group: Vec<Sort>) -> ExprFuncBuilder;
 }
 
 #[derive(Debug, Clone)]
@@ -791,7 +789,6 @@ pub struct ExprFuncBuilder {
     null_treatment: Option<NullTreatment>,
     partition_by: Option<Vec<Expr>>,
     window_frame: Option<WindowFrame>,
-    within_group: Option<Vec<Sort>>,
 }
 
 impl ExprFuncBuilder {
@@ -805,7 +802,6 @@ impl ExprFuncBuilder {
             null_treatment: None,
             partition_by: None,
             window_frame: None,
-            within_group: None,
         }
     }
 
@@ -824,7 +820,6 @@ impl ExprFuncBuilder {
             null_treatment,
             partition_by,
             window_frame,
-            within_group,
         } = self;
 
         let Some(fun) = fun else {
@@ -839,7 +834,6 @@ impl ExprFuncBuilder {
                 udaf.filter = filter.map(Box::new);
                 udaf.distinct = distinct;
                 udaf.null_treatment = null_treatment;
-                udaf.within_group = within_group;
                 Expr::AggregateFunction(udaf)
             }
             ExprFuncKind::Window(mut udwf) => {
@@ -894,11 +888,6 @@ impl ExprFunctionExt for ExprFuncBuilder {
         self.window_frame = Some(window_frame);
         self
     }
-
-    fn within_group(mut self, within_group: Vec<Sort>) -> ExprFuncBuilder {
-        self.within_group = Some(within_group);
-        self
-    }
 }
 
 impl ExprFunctionExt for Expr {
@@ -979,19 +968,6 @@ impl ExprFunctionExt for Expr {
             _ => ExprFuncBuilder::new(None),
         }
     }
-
-    fn within_group(self, within_group: Vec<Sort>) -> ExprFuncBuilder {
-        let mut builder = match self {
-            Expr::AggregateFunction(udaf) => {
-                ExprFuncBuilder::new(Some(ExprFuncKind::Aggregate(udaf)))
-            }
-            _ => ExprFuncBuilder::new(None),
-        };
-        if builder.fun.is_some() {
-            builder.within_group = Some(within_group);
-        }
-        builder
-    }
 }
 
 #[cfg(test)]
diff --git a/datafusion/expr/src/test/function_stub.rs b/datafusion/expr/src/test/function_stub.rs
index f71046a2669a3..71ab1ad6ef9b2 100644
--- a/datafusion/expr/src/test/function_stub.rs
+++ b/datafusion/expr/src/test/function_stub.rs
@@ -62,7 +62,6 @@ pub fn sum(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
     ))
 }
 
@@ -76,7 +75,6 @@ pub fn count(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
     ))
 }
 
@@ -90,7 +88,6 @@ pub fn avg(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
     ))
 }
 
@@ -293,7 +290,6 @@ pub fn min(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
     ))
 }
 
@@ -379,7 +375,6 @@ pub fn max(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
     ))
 }
 
diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs
index 2602554232aac..afd7863c46c8b 100644
--- a/datafusion/expr/src/tree_node.rs
+++ b/datafusion/expr/src/tree_node.rs
@@ -87,13 +87,8 @@ impl TreeNode for Expr {
                           }) => (expr, low, high).apply_ref_elements(f),
             Expr::Case(Case { expr, when_then_expr, else_expr }) =>
                 (expr, when_then_expr, else_expr).apply_ref_elements(f),
-            Expr::AggregateFunction(AggregateFunction { args, filter, order_by, within_group, .. }) => {
-                if within_group.is_some() {
-                    (args, filter, within_group).apply_ref_elements(f)
-                } else {
-                    (args, filter, order_by).apply_ref_elements(f)
-                }
-            }
+            Expr::AggregateFunction(AggregateFunction { args, filter, order_by, .. }) =>
+                (args, filter, order_by).apply_ref_elements(f),
             Expr::WindowFunction(WindowFunction {
                                      args,
                                      partition_by,
@@ -252,38 +247,18 @@ impl TreeNode for Expr {
                 filter,
                 order_by,
                 null_treatment,
-                within_group,
-            }) => {
-                if within_group.is_some() {
-                    (args, filter, within_group).map_elements(f)?.map_data(
-                        |(new_args, new_filter, new_within_group)| {
-                            Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
-                                func,
-                                new_args,
-                                distinct,
-                                new_filter,
-                                order_by,
-                                null_treatment,
-                                new_within_group,
-                            )))
-                        },
-                    )?
-                } else {
-                    (args, filter, order_by).map_elements(f)?.map_data(
-                        |(new_args, new_filter, new_order_by)| {
-                            Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
-                                func,
-                                new_args,
-                                distinct,
-                                new_filter,
-                                new_order_by,
-                                null_treatment,
-                                within_group,
-                            )))
-                        },
-                    )?
-                }
-            }
+            }) => (args, filter, order_by).map_elements(f)?.map_data(
+                |(new_args, new_filter, new_order_by)| {
+                    Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
+                       func,
+                       new_args,
+                       distinct,
+                       new_filter,
+                       new_order_by,
+                       null_treatment,
+                   )))
+                },
+            )?,
             Expr::GroupingSet(grouping_set) => match grouping_set {
                 GroupingSet::Rollup(exprs) => exprs
                     .map_elements(f)?
diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index ef71f48b2fe89..d2aa4f7cca71d 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -152,7 +152,6 @@ impl AggregateUDF {
             None,
             None,
             None,
-            None,
         ))
     }
 
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index b4ad25842008e..5969608cf824d 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -76,7 +76,6 @@ pub fn approx_percentile_cont(
         None,
         None,
         None,
-        within_group,
     ))
 }
 
diff --git a/datafusion/functions-aggregate/src/count.rs b/datafusion/functions-aggregate/src/count.rs
index 56f5f7dc2d849..8d8f04f00700a 100644
--- a/datafusion/functions-aggregate/src/count.rs
+++ b/datafusion/functions-aggregate/src/count.rs
@@ -76,7 +76,7 @@ pub fn count_distinct(expr: Expr) -> Expr {
         None,
         None,
         None,
-        None,
+
     ))
 }
 
diff --git a/datafusion/functions-aggregate/src/macros.rs b/datafusion/functions-aggregate/src/macros.rs
index b0d7cb15d2461..b464dde6ccab5 100644
--- a/datafusion/functions-aggregate/src/macros.rs
+++ b/datafusion/functions-aggregate/src/macros.rs
@@ -29,7 +29,6 @@ macro_rules! make_udaf_expr {
                 None,
                 None,
                 None,
-                None,
             ))
         }
     };
@@ -53,7 +52,6 @@ macro_rules! make_udaf_expr_and_func {
                 None,
                 None,
                 None,
-                None,
             ))
         }
 
diff --git a/datafusion/functions-nested/src/planner.rs b/datafusion/functions-nested/src/planner.rs
index 7c4ff41a49755..5ca51ac20f1e5 100644
--- a/datafusion/functions-nested/src/planner.rs
+++ b/datafusion/functions-nested/src/planner.rs
@@ -163,7 +163,6 @@ impl ExprPlanner for FieldAccessPlanner {
                                 agg_func.filter,
                                 agg_func.order_by,
                                 agg_func.null_treatment,
-                                agg_func.within_group,
                             ),
                         )))
                     }
diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs
index dc76d29f4a6bb..85fc9b31bcddd 100644
--- a/datafusion/optimizer/src/analyzer/type_coercion.rs
+++ b/datafusion/optimizer/src/analyzer/type_coercion.rs
@@ -511,7 +511,6 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
                 filter,
                 order_by,
                 null_treatment,
-                within_group,
             }) => {
                 let new_expr = coerce_arguments_for_signature_with_aggregate_udf(
                     args,
@@ -526,7 +525,6 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
                         filter,
                         order_by,
                         null_treatment,
-                        within_group,
                     ),
                 )))
             }
@@ -1341,7 +1339,6 @@ mod test {
             None,
             None,
             None,
-            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![udaf], empty)?);
         let expected = "Projection: MY_AVG(CAST(Int64(10) AS Float64))\n  EmptyRelation";
@@ -1371,7 +1368,6 @@ mod test {
             None,
             None,
             None,
-            None,
         ));
 
         let err = Projection::try_new(vec![udaf], empty).err().unwrap();
@@ -1391,7 +1387,6 @@ mod test {
             None,
             None,
             None,
-            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![agg_expr], empty)?);
         let expected = "Projection: avg(Float64(12))\n  EmptyRelation";
@@ -1405,7 +1400,6 @@ mod test {
             None,
             None,
             None,
-            None,
         ));
         let plan = LogicalPlan::Projection(Projection::try_new(vec![agg_expr], empty)?);
         let expected = "Projection: avg(CAST(a AS Float64))\n  EmptyRelation";
@@ -1423,7 +1417,6 @@ mod test {
             None,
             None,
             None,
-            None,
         ));
         let err = Projection::try_new(vec![agg_expr], empty)
             .err()
diff --git a/datafusion/optimizer/src/common_subexpr_eliminate.rs b/datafusion/optimizer/src/common_subexpr_eliminate.rs
index 9a4646ac6bf4d..4b9a83fd3e4c0 100644
--- a/datafusion/optimizer/src/common_subexpr_eliminate.rs
+++ b/datafusion/optimizer/src/common_subexpr_eliminate.rs
@@ -891,7 +891,6 @@ mod test {
                 None,
                 None,
                 None,
-                None,
             ))
         };
 
diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
index cd93b642af191..29f3d7cbda395 100644
--- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
+++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
@@ -4020,7 +4020,6 @@ mod tests {
                 None,
                 None,
                 None,
-                None,
             ));
 
         let expected = col("result_column");
@@ -4035,7 +4034,6 @@ mod tests {
                 None,
                 None,
                 None,
-                None,
             ));
 
         let expected = aggregate_function_expr.clone();
diff --git a/datafusion/optimizer/src/single_distinct_to_groupby.rs b/datafusion/optimizer/src/single_distinct_to_groupby.rs
index 6fa60428cd80b..34f0d1a03d508 100644
--- a/datafusion/optimizer/src/single_distinct_to_groupby.rs
+++ b/datafusion/optimizer/src/single_distinct_to_groupby.rs
@@ -73,10 +73,9 @@ fn is_single_distinct_agg(aggr_expr: &[Expr]) -> Result<bool> {
             filter,
             order_by,
             null_treatment: _,
-            within_group,
         }) = expr
         {
-            if filter.is_some() || order_by.is_some() || within_group.is_some() {
+            if filter.is_some() || order_by.is_some() {
                 return Ok(false);
             }
             aggregate_count += 1;
@@ -201,7 +200,6 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                     None,
                                     None,
                                     None,
-                                    None,
                                 )))
                                 // if the aggregate function is not distinct, we need to rewrite it like two phase aggregation
                             } else {
@@ -215,7 +213,6 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                         None,
                                         None,
                                         None,
-                                        None,
                                     ))
                                     .alias(&alias_str),
                                 );
@@ -226,7 +223,6 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                     None,
                                     None,
                                     None,
-                                    None,
                                 )))
                             }
                         }
@@ -299,7 +295,6 @@ mod tests {
             None,
             None,
             None,
-            None,
         ))
     }
 
@@ -625,7 +620,6 @@ mod tests {
             None,
             Some(vec![col("a").sort(true, false)]),
             None,
-            None,
         ));
         let plan = LogicalPlanBuilder::from(table_scan)
             .aggregate(vec![col("c")], vec![expr, count_distinct(col("b"))])?
diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs
index 72d3fc1d54a01..e04a89a03dae5 100644
--- a/datafusion/proto/src/logical_plan/from_proto.rs
+++ b/datafusion/proto/src/logical_plan/from_proto.rs
@@ -562,10 +562,6 @@ pub fn parse_expr(
                     _ => Some(parse_sorts(&pb.order_by, registry, codec)?),
                 },
                 None,
-                match pb.within_group.len() {
-                    0 => None,
-                    _ => Some(parse_sorts(&pb.within_group, registry, codec)?),
-                },
             )))
         }
 
diff --git a/datafusion/proto/src/logical_plan/to_proto.rs b/datafusion/proto/src/logical_plan/to_proto.rs
index 6898c7962cf07..6d1d4f30610c3 100644
--- a/datafusion/proto/src/logical_plan/to_proto.rs
+++ b/datafusion/proto/src/logical_plan/to_proto.rs
@@ -353,7 +353,6 @@ pub fn serialize_expr(
             ref filter,
             ref order_by,
             null_treatment: _,
-            ref within_group,
         }) => {
             let mut buf = Vec::new();
             let _ = codec.try_encode_udaf(func, &mut buf);
@@ -372,10 +371,6 @@ pub fn serialize_expr(
                             None => vec![],
                         },
                         fun_definition: (!buf.is_empty()).then_some(buf),
-                        within_group: match within_group {
-                            Some(e) => serialize_sorts(e, codec)?,
-                            None => vec![],
-                        },
                     },
                 ))),
             }
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index dd83f384b9dff..10ab23ee75f39 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -2242,7 +2242,6 @@ fn roundtrip_aggregate_udf() {
         Some(Box::new(lit(true))),
         None,
         None,
-        None,
     ));
 
     let ctx = SessionContext::new();
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index f75e811ff777f..296e524b7d233 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -402,7 +402,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                     filter,
                     order_by,
                     null_treatment,
-                    within_group,
                 )));
             }
         }
diff --git a/datafusion/substrait/src/logical_plan/consumer.rs b/datafusion/substrait/src/logical_plan/consumer.rs
index 69dc5662261f9..481e3cc6f2a16 100644
--- a/datafusion/substrait/src/logical_plan/consumer.rs
+++ b/datafusion/substrait/src/logical_plan/consumer.rs
@@ -1221,7 +1221,7 @@ pub async fn from_aggregate_rel(
                         _ => false,
                     };
 
-                    let sorts = if !f.sorts.is_empty() {
+                    let order_by = if !f.sorts.is_empty() {
                         Some(
                             from_substrait_sorts(consumer, &f.sorts, input.schema())
                                 .await?,
@@ -1235,7 +1235,7 @@ pub async fn from_aggregate_rel(
                         f,
                         input.schema(),
                         filter,
-                        sorts,
+                        order_by,
                         distinct,
                     )
                     .await
@@ -1919,7 +1919,7 @@ pub async fn from_substrait_agg_func(
     f: &AggregateFunction,
     input_schema: &DFSchema,
     filter: Option<Box<Expr>>,
-    sorts: Option<Vec<SortExpr>>,
+    order_by: Option<Vec<SortExpr>>,
     distinct: bool,
 ) -> Result<Arc<Expr>> {
     let Some(fn_signature) = consumer
@@ -1952,12 +1952,6 @@ pub async fn from_substrait_agg_func(
         args
     };
 
-    let (within_group, order_by) =
-        match consumer.get_function_registry().ordered_set_udaf(fn_name) {
-            Ok(_) => (sorts, None),
-            Err(_) => (None, sorts),
-        };
-
     Ok(Arc::new(Expr::AggregateFunction(
         expr::AggregateFunction::new_udf(
             udaf,
@@ -1966,7 +1960,6 @@ pub async fn from_substrait_agg_func(
             filter,
             order_by,
             None,
-            within_group,
         ),
     )))
 }
diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs
index fc1c0c98910f3..9546a2e1b1968 100644
--- a/datafusion/substrait/src/logical_plan/producer.rs
+++ b/datafusion/substrait/src/logical_plan/producer.rs
@@ -1213,19 +1213,13 @@ pub fn from_aggregate_function(
         filter,
         order_by,
         null_treatment: _null_treatment,
-        within_group,
     } = agg_fn;
 
-    let sorts = match (within_group, order_by) {
-        (Some(within_group), _) => within_group
-            .iter()
-            .map(|expr| to_substrait_sort_field(producer, expr, schema))
-            .collect::<Result<Vec<_>>>()?,
-        (None, Some(order_by)) => order_by
-            .iter()
-            .map(|expr| to_substrait_sort_field(producer, expr, schema))
-            .collect::<Result<Vec<_>>>()?,
-        (None, None) => vec![],
+    let sorts = if let Some(order_by) = order_by {
+        order_by
+            .collect::<Result<Vec<_>>>()?
+    } else {
+        vec![]
     };
 
     let mut arguments: Vec<FunctionArgument> = vec![];

From d482bff7d695688438465cd4c11d45c743250201 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Thu, 27 Feb 2025 17:43:27 +0900
Subject: [PATCH 21/39] Rollback ordered set aggregate functions from session
 to save same info in udf itself

---
 datafusion-examples/examples/sql_frontend.rs  |   4 -
 datafusion/catalog-listing/src/helpers.rs     |   6 -
 datafusion/catalog/src/session.rs             |  16 +--
 datafusion/core/src/execution/context/mod.rs  |  13 --
 .../core/src/execution/session_state.rs       | 118 +-----------------
 .../src/execution/session_state_defaults.rs   |   7 --
 datafusion/core/tests/optimizer/mod.rs        |   4 -
 datafusion/execution/src/lib.rs               |   2 +-
 datafusion/execution/src/task.rs              |  75 ++---------
 datafusion/expr/src/planner.rs                |   4 -
 datafusion/expr/src/registry.rs               |  52 --------
 datafusion/functions-aggregate/src/lib.rs     |  10 --
 .../optimizer/tests/optimizer_integration.rs  |   5 -
 datafusion/proto/src/bytes/mod.rs             |  19 ---
 datafusion/proto/src/bytes/registry.rs        |  10 --
 datafusion/sql/examples/sql.rs                |   6 -
 datafusion/sql/src/expr/mod.rs                |   7 --
 datafusion/sql/tests/common/mod.rs            |   8 --
 18 files changed, 18 insertions(+), 348 deletions(-)

diff --git a/datafusion-examples/examples/sql_frontend.rs b/datafusion-examples/examples/sql_frontend.rs
index eefafaa31bbf4..3955d5038cfb0 100644
--- a/datafusion-examples/examples/sql_frontend.rs
+++ b/datafusion-examples/examples/sql_frontend.rs
@@ -157,10 +157,6 @@ impl ContextProvider for MyContextProvider {
         None
     }
 
-    fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
-        None
-    }
-
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/catalog-listing/src/helpers.rs b/datafusion/catalog-listing/src/helpers.rs
index 1a7525beb1f89..6cb3f661e652f 100644
--- a/datafusion/catalog-listing/src/helpers.rs
+++ b/datafusion/catalog-listing/src/helpers.rs
@@ -1054,12 +1054,6 @@ mod tests {
             unimplemented!()
         }
 
-        fn ordered_set_aggregate_functions(
-            &self,
-        ) -> &std::collections::HashMap<String, Arc<AggregateUDF>> {
-            unimplemented!()
-        }
-
         fn window_functions(&self) -> &std::collections::HashMap<String, Arc<WindowUDF>> {
             unimplemented!()
         }
diff --git a/datafusion/catalog/src/session.rs b/datafusion/catalog/src/session.rs
index 4ef458aab7338..db49529ac43f5 100644
--- a/datafusion/catalog/src/session.rs
+++ b/datafusion/catalog/src/session.rs
@@ -20,7 +20,7 @@ use datafusion_common::config::ConfigOptions;
 use datafusion_common::{DFSchema, Result};
 use datafusion_execution::config::SessionConfig;
 use datafusion_execution::runtime_env::RuntimeEnv;
-use datafusion_execution::{TaskContext, TaskContextFunctionParams};
+use datafusion_execution::TaskContext;
 use datafusion_expr::execution_props::ExecutionProps;
 use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
 use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
@@ -110,11 +110,6 @@ pub trait Session: Send + Sync {
     /// Return reference to aggregate_functions
     fn aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>>;
 
-    /// Return reference to ordered_set_aggregate_functions
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>>;
-
     /// Return reference to window functions
     fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>>;
 
@@ -135,12 +130,9 @@ impl From<&dyn Session> for TaskContext {
             task_id,
             state.session_id().to_string(),
             state.config().clone(),
-            TaskContextFunctionParams::new(
-                state.scalar_functions().clone(),
-                state.aggregate_functions().clone(),
-                state.ordered_set_aggregate_functions().clone(),
-                state.window_functions().clone(),
-            ),
+            state.scalar_functions().clone(),
+            state.aggregate_functions().clone(),
+            state.window_functions().clone(),
             state.runtime_env().clone(),
         )
     }
diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs
index 90a25d5c5f50a..61ddba10b09cc 100644
--- a/datafusion/core/src/execution/context/mod.rs
+++ b/datafusion/core/src/execution/context/mod.rs
@@ -1604,10 +1604,6 @@ impl FunctionRegistry for SessionContext {
         self.state.read().udaf(name)
     }
 
-    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-        self.state.read().ordered_set_udaf(name)
-    }
-
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         self.state.read().udwf(name)
     }
@@ -1623,15 +1619,6 @@ impl FunctionRegistry for SessionContext {
         self.state.write().register_udaf(udaf)
     }
 
-    fn register_ordered_set_udaf(
-        &mut self,
-        ordered_set_udaf: Arc<AggregateUDF>,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        self.state
-            .write()
-            .register_ordered_set_udaf(ordered_set_udaf)
-    }
-
     fn register_udwf(&mut self, udwf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         self.state.write().register_udwf(udwf)
     }
diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs
index 5573b06d33cc8..4f752dc69be5d 100644
--- a/datafusion/core/src/execution/session_state.rs
+++ b/datafusion/core/src/execution/session_state.rs
@@ -48,7 +48,7 @@ use datafusion_common::{
 };
 use datafusion_execution::config::SessionConfig;
 use datafusion_execution::runtime_env::RuntimeEnv;
-use datafusion_execution::{TaskContext, TaskContextFunctionParams};
+use datafusion_execution::TaskContext;
 use datafusion_expr::execution_props::ExecutionProps;
 use datafusion_expr::expr_rewriter::FunctionRewrite;
 use datafusion_expr::planner::{ExprPlanner, TypePlanner};
@@ -146,10 +146,6 @@ pub struct SessionState {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate functions registered in the context
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-    /// Ordered-set aggregate functions registered in the context
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     /// Window functions registered in the context
     window_functions: HashMap<String, Arc<WindowUDF>>,
     /// Deserializer registry for extensions.
@@ -208,10 +204,6 @@ impl Debug for SessionState {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
-            .field(
-                "ordered_set_aggregate_functions",
-                &self.ordered_set_aggregate_functions,
-            )
             .field("window_functions", &self.window_functions)
             .field("prepared_plans", &self.prepared_plans)
             .finish()
@@ -251,10 +243,6 @@ impl Session for SessionState {
         &self.aggregate_functions
     }
 
-    fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
-        &self.ordered_set_aggregate_functions
-    }
-
     fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
     }
@@ -906,13 +894,6 @@ impl SessionState {
         &self.aggregate_functions
     }
 
-    /// Return reference to ordered_set_aggregate_functions
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    pub fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
-        &self.ordered_set_aggregate_functions
-    }
-
     /// Return reference to window functions
     pub fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
@@ -1001,7 +982,6 @@ pub struct SessionStateBuilder {
     table_functions: Option<HashMap<String, Arc<TableFunction>>>,
     scalar_functions: Option<Vec<Arc<ScalarUDF>>>,
     aggregate_functions: Option<Vec<Arc<AggregateUDF>>>,
-    ordered_set_aggregate_functions: Option<Vec<Arc<AggregateUDF>>>,
     window_functions: Option<Vec<Arc<WindowUDF>>>,
     serializer_registry: Option<Arc<dyn SerializerRegistry>>,
     file_formats: Option<Vec<Arc<dyn FileFormatFactory>>>,
@@ -1032,7 +1012,6 @@ impl SessionStateBuilder {
             table_functions: None,
             scalar_functions: None,
             aggregate_functions: None,
-            ordered_set_aggregate_functions: None,
             window_functions: None,
             serializer_registry: None,
             file_formats: None,
@@ -1083,12 +1062,6 @@ impl SessionStateBuilder {
             aggregate_functions: Some(
                 existing.aggregate_functions.into_values().collect_vec(),
             ),
-            ordered_set_aggregate_functions: Some(
-                existing
-                    .ordered_set_aggregate_functions
-                    .into_values()
-                    .collect_vec(),
-            ),
             window_functions: Some(existing.window_functions.into_values().collect_vec()),
             serializer_registry: Some(existing.serializer_registry),
             file_formats: Some(existing.file_formats.into_values().collect_vec()),
@@ -1114,9 +1087,6 @@ impl SessionStateBuilder {
             .with_expr_planners(SessionStateDefaults::default_expr_planners())
             .with_scalar_functions(SessionStateDefaults::default_scalar_functions())
             .with_aggregate_functions(SessionStateDefaults::default_aggregate_functions())
-            .with_ordered_set_aggregate_functions(
-                SessionStateDefaults::default_ordered_set_aggregate_functions(),
-            )
             .with_window_functions(SessionStateDefaults::default_window_functions())
             .with_table_function_list(SessionStateDefaults::default_table_functions())
     }
@@ -1264,17 +1234,6 @@ impl SessionStateBuilder {
         self
     }
 
-    /// Set the map of ordered-set [`AggregateUDF`]s
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    pub fn with_ordered_set_aggregate_functions(
-        mut self,
-        ordered_set_aggregate_functions: Vec<Arc<AggregateUDF>>,
-    ) -> Self {
-        self.ordered_set_aggregate_functions = Some(ordered_set_aggregate_functions);
-        self
-    }
-
     /// Set the map of [`WindowUDF`]s
     pub fn with_window_functions(
         mut self,
@@ -1409,7 +1368,6 @@ impl SessionStateBuilder {
             table_functions,
             scalar_functions,
             aggregate_functions,
-            ordered_set_aggregate_functions,
             window_functions,
             serializer_registry,
             file_formats,
@@ -1441,7 +1399,6 @@ impl SessionStateBuilder {
             table_functions: table_functions.unwrap_or_default(),
             scalar_functions: HashMap::new(),
             aggregate_functions: HashMap::new(),
-            ordered_set_aggregate_functions: HashMap::new(),
             window_functions: HashMap::new(),
             serializer_registry: serializer_registry
                 .unwrap_or(Arc::new(EmptySerializerRegistry)),
@@ -1482,20 +1439,6 @@ impl SessionStateBuilder {
             });
         }
 
-        if let Some(ordered_set_aggregate_functions) = ordered_set_aggregate_functions {
-            ordered_set_aggregate_functions
-                .into_iter()
-                .for_each(|udaf| {
-                    let existing_ordered_set_udf = state.register_ordered_set_udaf(udaf);
-                    if let Ok(Some(existing_ordered_set_udf)) = existing_ordered_set_udf {
-                        debug!(
-                            "Overwrote an ordered set existing UDF: {}",
-                            existing_ordered_set_udf.name()
-                        );
-                    }
-                });
-        }
-
         if let Some(window_functions) = window_functions {
             window_functions.into_iter().for_each(|udwf| {
                 let existing_udf = state.register_udwf(udwf);
@@ -1695,10 +1638,6 @@ impl Debug for SessionStateBuilder {
             .field("table_functions", &self.table_functions)
             .field("scalar_functions", &self.scalar_functions)
             .field("aggregate_functions", &self.aggregate_functions)
-            .field(
-                "ordered_set_aggregate_functions",
-                &self.ordered_set_aggregate_functions,
-            )
             .field("window_functions", &self.window_functions)
             .finish()
     }
@@ -1785,13 +1724,6 @@ impl ContextProvider for SessionContextProvider<'_> {
         self.state.aggregate_functions().get(name).cloned()
     }
 
-    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
-        self.state
-            .ordered_set_aggregate_functions()
-            .get(name)
-            .cloned()
-    }
-
     fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>> {
         self.state.window_functions().get(name).cloned()
     }
@@ -1862,19 +1794,6 @@ impl FunctionRegistry for SessionState {
         })
     }
 
-    fn ordered_set_udaf(
-        &self,
-        name: &str,
-    ) -> datafusion_common::Result<Arc<AggregateUDF>> {
-        let result = self.ordered_set_aggregate_functions.get(name);
-
-        result.cloned().ok_or_else(|| {
-            plan_datafusion_err!(
-                "There is no ordered set UDAF named \"{name}\" in the registry"
-            )
-        })
-    }
-
     fn udwf(&self, name: &str) -> datafusion_common::Result<Arc<WindowUDF>> {
         let result = self.window_functions.get(name);
 
@@ -1905,19 +1824,6 @@ impl FunctionRegistry for SessionState {
         Ok(self.aggregate_functions.insert(udaf.name().into(), udaf))
     }
 
-    fn register_ordered_set_udaf(
-        &mut self,
-        ordered_set_udaf: Arc<AggregateUDF>,
-    ) -> datafusion_common::Result<Option<Arc<AggregateUDF>>> {
-        ordered_set_udaf.aliases().iter().for_each(|alias| {
-            self.ordered_set_aggregate_functions
-                .insert(alias.clone(), Arc::clone(&ordered_set_udaf));
-        });
-        Ok(self
-            .ordered_set_aggregate_functions
-            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
-    }
-
     fn register_udwf(
         &mut self,
         udwf: Arc<WindowUDF>,
@@ -1955,19 +1861,6 @@ impl FunctionRegistry for SessionState {
         Ok(udaf)
     }
 
-    fn deregister_ordered_set_udaf(
-        &mut self,
-        name: &str,
-    ) -> datafusion_common::Result<Option<Arc<AggregateUDF>>> {
-        let ordered_set_udaf = self.ordered_set_aggregate_functions.remove(name);
-        if let Some(ordered_set_udaf) = &ordered_set_udaf {
-            for alias in ordered_set_udaf.aliases() {
-                self.ordered_set_aggregate_functions.remove(alias);
-            }
-        }
-        Ok(ordered_set_udaf)
-    }
-
     fn deregister_udwf(
         &mut self,
         name: &str,
@@ -2028,12 +1921,9 @@ impl From<&SessionState> for TaskContext {
             task_id,
             state.session_id.clone(),
             state.config.clone(),
-            TaskContextFunctionParams::new(
-                state.scalar_functions.clone(),
-                state.aggregate_functions.clone(),
-                state.ordered_set_aggregate_functions.clone(),
-                state.window_functions.clone(),
-            ),
+            state.scalar_functions.clone(),
+            state.aggregate_functions.clone(),
+            state.window_functions.clone(),
             Arc::clone(&state.runtime_env),
         )
     }
diff --git a/datafusion/core/src/execution/session_state_defaults.rs b/datafusion/core/src/execution/session_state_defaults.rs
index 161ba8f60d280..92f649781cfd9 100644
--- a/datafusion/core/src/execution/session_state_defaults.rs
+++ b/datafusion/core/src/execution/session_state_defaults.rs
@@ -115,13 +115,6 @@ impl SessionStateDefaults {
         functions_aggregate::all_default_aggregate_functions()
     }
 
-    /// returns the list of default ordered-set [`AggregateUDF']'s
-    ///
-    /// Note : default_ordered_set_aggregate_functions are a subset of default_aggregate_functions.
-    pub fn default_ordered_set_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
-        functions_aggregate::all_default_ordered_set_aggregate_functions()
-    }
-
     /// returns the list of default [`WindowUDF']'s
     pub fn default_window_functions() -> Vec<Arc<WindowUDF>> {
         functions_window::all_default_window_functions()
diff --git a/datafusion/core/tests/optimizer/mod.rs b/datafusion/core/tests/optimizer/mod.rs
index 793565153e6ef..37a6ca7f5934e 100644
--- a/datafusion/core/tests/optimizer/mod.rs
+++ b/datafusion/core/tests/optimizer/mod.rs
@@ -201,10 +201,6 @@ impl ContextProvider for MyContextProvider {
         None
     }
 
-    fn get_ordered_set_aggregate_meta(&self, _name: &str) -> Option<Arc<AggregateUDF>> {
-        None
-    }
-
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/execution/src/lib.rs b/datafusion/execution/src/lib.rs
index 6340d774535ed..317bd3203ab1b 100644
--- a/datafusion/execution/src/lib.rs
+++ b/datafusion/execution/src/lib.rs
@@ -38,4 +38,4 @@ pub mod registry {
 pub use disk_manager::DiskManager;
 pub use registry::FunctionRegistry;
 pub use stream::{RecordBatchStream, SendableRecordBatchStream};
-pub use task::{TaskContext, TaskContextFunctionParams};
+pub use task::TaskContext;
diff --git a/datafusion/execution/src/task.rs b/datafusion/execution/src/task.rs
index d031c52127a28..7cdb53c90d0ef 100644
--- a/datafusion/execution/src/task.rs
+++ b/datafusion/execution/src/task.rs
@@ -44,10 +44,6 @@ pub struct TaskContext {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate functions associated with this task context
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-    /// Ordered set aggregate functions associated with this task context
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     /// Window functions associated with this task context
     window_functions: HashMap<String, Arc<WindowUDF>>,
     /// Runtime environment associated with this task context
@@ -65,7 +61,6 @@ impl Default for TaskContext {
             session_config: SessionConfig::new(),
             scalar_functions: HashMap::new(),
             aggregate_functions: HashMap::new(),
-            ordered_set_aggregate_functions: HashMap::new(),
             window_functions: HashMap::new(),
             runtime,
         }
@@ -82,18 +77,18 @@ impl TaskContext {
         task_id: Option<String>,
         session_id: String,
         session_config: SessionConfig,
-        function_params: TaskContextFunctionParams,
+        scalar_functions: HashMap<String, Arc<ScalarUDF>>,
+        aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
+        window_functions: HashMap<String, Arc<WindowUDF>>,
         runtime: Arc<RuntimeEnv>,
     ) -> Self {
         Self {
             task_id,
             session_id,
             session_config,
-            scalar_functions: function_params.scalar_functions,
-            aggregate_functions: function_params.aggregate_functions,
-            ordered_set_aggregate_functions: function_params
-                .ordered_set_aggregate_functions,
-            window_functions: function_params.window_functions,
+            scalar_functions,
+            aggregate_functions,
+            window_functions,
             runtime,
         }
     }
@@ -131,10 +126,6 @@ impl TaskContext {
         &self.aggregate_functions
     }
 
-    pub fn ordered_set_aggregate_functions(&self) -> &HashMap<String, Arc<AggregateUDF>> {
-        &self.ordered_set_aggregate_functions
-    }
-
     pub fn window_functions(&self) -> &HashMap<String, Arc<WindowUDF>> {
         &self.window_functions
     }
@@ -152,29 +143,6 @@ impl TaskContext {
     }
 }
 
-pub struct TaskContextFunctionParams {
-    scalar_functions: HashMap<String, Arc<ScalarUDF>>,
-    aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-    window_functions: HashMap<String, Arc<WindowUDF>>,
-}
-
-impl TaskContextFunctionParams {
-    pub fn new(
-        scalar_functions: HashMap<String, Arc<ScalarUDF>>,
-        aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-        ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-        window_functions: HashMap<String, Arc<WindowUDF>>,
-    ) -> Self {
-        Self {
-            scalar_functions,
-            aggregate_functions,
-            ordered_set_aggregate_functions,
-            window_functions,
-        }
-    }
-}
-
 impl FunctionRegistry for TaskContext {
     fn udfs(&self) -> HashSet<String> {
         self.scalar_functions.keys().cloned().collect()
@@ -196,16 +164,6 @@ impl FunctionRegistry for TaskContext {
         })
     }
 
-    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-        let result = self.ordered_set_aggregate_functions.get(name);
-
-        result.cloned().ok_or_else(|| {
-            plan_datafusion_err!(
-                "There is no ordered set UDAF named \"{name}\" in the TaskContext"
-            )
-        })
-    }
-
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         let result = self.window_functions.get(name);
 
@@ -225,18 +183,6 @@ impl FunctionRegistry for TaskContext {
         });
         Ok(self.aggregate_functions.insert(udaf.name().into(), udaf))
     }
-    fn register_ordered_set_udaf(
-        &mut self,
-        ordered_set_udaf: Arc<AggregateUDF>,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        ordered_set_udaf.aliases().iter().for_each(|alias| {
-            self.ordered_set_aggregate_functions
-                .insert(alias.clone(), Arc::clone(&ordered_set_udaf));
-        });
-        Ok(self
-            .ordered_set_aggregate_functions
-            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
-    }
     fn register_udwf(&mut self, udwf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         udwf.aliases().iter().for_each(|alias| {
             self.window_functions
@@ -289,12 +235,9 @@ mod tests {
             Some("task_id".to_string()),
             "session_id".to_string(),
             session_config,
-            TaskContextFunctionParams::new(
-                HashMap::default(),
-                HashMap::default(),
-                HashMap::default(),
-                HashMap::default(),
-            ),
+            HashMap::default(),
+            HashMap::default(),
+            HashMap::default(),
             runtime,
         );
 
diff --git a/datafusion/expr/src/planner.rs b/datafusion/expr/src/planner.rs
index fa4a4f5025f7a..42047e8e6caa2 100644
--- a/datafusion/expr/src/planner.rs
+++ b/datafusion/expr/src/planner.rs
@@ -76,10 +76,6 @@ pub trait ContextProvider {
     fn get_function_meta(&self, name: &str) -> Option<Arc<ScalarUDF>>;
     /// Getter for a UDAF description
     fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
-    /// Getter for a ordered-set UDAF description
-    ///
-    /// Note : ordered_set_aggregate_meta is a subset of aggregate_meta.
-    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
     /// Getter for a UDWF
     fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>>;
     /// Getter for system/user-defined variable type
diff --git a/datafusion/expr/src/registry.rs b/datafusion/expr/src/registry.rs
index 609aa096dde19..35a002a952bbe 100644
--- a/datafusion/expr/src/registry.rs
+++ b/datafusion/expr/src/registry.rs
@@ -38,12 +38,6 @@ pub trait FunctionRegistry {
     /// `name`.
     fn udaf(&self, name: &str) -> Result<Arc<AggregateUDF>>;
 
-    /// Returns a reference to the user defined ordered set aggregate function (udaf) named
-    /// `name`.
-    ///
-    /// Note : ordered_set_aggregate_functions are a subset of aggregate_functions.
-    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>>;
-
     /// Returns a reference to the user defined window function (udwf) named
     /// `name`.
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>>;
@@ -68,20 +62,6 @@ pub trait FunctionRegistry {
         not_impl_err!("Registering AggregateUDF")
     }
 
-    /// Registers a new ordered-set [`AggregateUDF`], returning any previously registered
-    /// implementation.
-    ///
-    /// Returns an error (the default) if the function can not be registered,
-    /// for example if the registry is read only.
-    ///
-    /// Note : ordered_set_udaf is a subset of udaf.
-    fn register_ordered_set_udaf(
-        &mut self,
-        _udaf: Arc<AggregateUDF>,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        not_impl_err!("Registering ordered-set AggregateUDF")
-    }
-
     /// Registers a new [`WindowUDF`], returning any previously registered
     /// implementation.
     ///
@@ -109,20 +89,6 @@ pub trait FunctionRegistry {
         not_impl_err!("Deregistering AggregateUDF")
     }
 
-    /// Deregisters a ordered set [`AggregateUDF`], returning the implementation that was
-    /// deregistered.
-    ///
-    /// Returns an error (the default) if the function can not be deregistered,
-    /// for example if the registry is read only.
-    ///
-    /// Note : ordered_set_udaf is a subset of udaf.
-    fn deregister_ordered_set_udaf(
-        &mut self,
-        _name: &str,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        not_impl_err!("Deregistering ordered set AggregateUDF")
-    }
-
     /// Deregisters a [`WindowUDF`], returning the implementation that was
     /// deregistered.
     ///
@@ -183,10 +149,6 @@ pub struct MemoryFunctionRegistry {
     udfs: HashMap<String, Arc<ScalarUDF>>,
     /// Aggregate Functions
     udafs: HashMap<String, Arc<AggregateUDF>>,
-    /// Ordered Set Aggregate Functions
-    ///
-    /// Note : ordered_set_udafs are a subset of udafs.
-    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
     /// Window Functions
     udwfs: HashMap<String, Arc<WindowUDF>>,
 }
@@ -216,12 +178,6 @@ impl FunctionRegistry for MemoryFunctionRegistry {
             .ok_or_else(|| plan_datafusion_err!("Aggregate Function {name} not found"))
     }
 
-    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-        self.ordered_set_udafs.get(name).cloned().ok_or_else(|| {
-            plan_datafusion_err!("Ordered Set Aggregate Function {name} not found")
-        })
-    }
-
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         self.udwfs
             .get(name)
@@ -238,14 +194,6 @@ impl FunctionRegistry for MemoryFunctionRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         Ok(self.udafs.insert(udaf.name().into(), udaf))
     }
-    fn register_ordered_set_udaf(
-        &mut self,
-        ordered_set_udaf: Arc<AggregateUDF>,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        Ok(self
-            .ordered_set_udafs
-            .insert(ordered_set_udaf.name().into(), ordered_set_udaf))
-    }
     fn register_udwf(&mut self, udaf: Arc<WindowUDF>) -> Result<Option<Arc<WindowUDF>>> {
         Ok(self.udwfs.insert(udaf.name().into(), udaf))
     }
diff --git a/datafusion/functions-aggregate/src/lib.rs b/datafusion/functions-aggregate/src/lib.rs
index b0e9079b69ac5..746873442d9aa 100644
--- a/datafusion/functions-aggregate/src/lib.rs
+++ b/datafusion/functions-aggregate/src/lib.rs
@@ -174,16 +174,6 @@ pub fn all_default_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
     ]
 }
 
-/// Returns all default ordered set aggregate functions
-///
-/// Note : all_default_ordered_set_aggregate_functions are a subset of all_default_aggregate_functions.
-pub fn all_default_ordered_set_aggregate_functions() -> Vec<Arc<AggregateUDF>> {
-    vec![
-        approx_percentile_cont_udaf(),
-        approx_percentile_cont_with_weight_udaf(),
-    ]
-}
-
 /// Registers all enabled packages with a [`FunctionRegistry`]
 pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
     let functions: Vec<Arc<AggregateUDF>> = all_default_aggregate_functions();
diff --git a/datafusion/optimizer/tests/optimizer_integration.rs b/datafusion/optimizer/tests/optimizer_integration.rs
index ae62f42892c41..a33ecbc3a1fb3 100644
--- a/datafusion/optimizer/tests/optimizer_integration.rs
+++ b/datafusion/optimizer/tests/optimizer_integration.rs
@@ -440,7 +440,6 @@ fn observe(_plan: &LogicalPlan, _rule: &dyn OptimizerRule) {}
 struct MyContextProvider {
     options: ConfigOptions,
     udafs: HashMap<String, Arc<AggregateUDF>>,
-    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
 }
 
 impl MyContextProvider {
@@ -494,10 +493,6 @@ impl ContextProvider for MyContextProvider {
         self.udafs.get(name).cloned()
     }
 
-    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
-        self.ordered_set_udafs.get(name).cloned()
-    }
-
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/proto/src/bytes/mod.rs b/datafusion/proto/src/bytes/mod.rs
index 2d56c86975846..12ddb4cb2e329 100644
--- a/datafusion/proto/src/bytes/mod.rs
+++ b/datafusion/proto/src/bytes/mod.rs
@@ -133,17 +133,6 @@ impl Serializeable for Expr {
                 )))
             }
 
-            fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-                Ok(Arc::new(create_udaf(
-                    name,
-                    vec![arrow::datatypes::DataType::Null],
-                    Arc::new(arrow::datatypes::DataType::Null),
-                    Volatility::Immutable,
-                    Arc::new(|_| unimplemented!()),
-                    Arc::new(vec![]),
-                )))
-            }
-
             fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
                 Ok(Arc::new(create_udwf(
                     name,
@@ -161,14 +150,6 @@ impl Serializeable for Expr {
                     "register_udaf called in Placeholder Registry!"
                 )
             }
-            fn register_ordered_set_udaf(
-                &mut self,
-                _udaf: Arc<AggregateUDF>,
-            ) -> Result<Option<Arc<AggregateUDF>>> {
-                datafusion_common::internal_err!(
-                    "register_ordered_set_udaf called in Placeholder Registry!"
-                )
-            }
             fn register_udf(
                 &mut self,
                 _udf: Arc<datafusion_expr::ScalarUDF>,
diff --git a/datafusion/proto/src/bytes/registry.rs b/datafusion/proto/src/bytes/registry.rs
index d14f58b651f7c..eae2425f8ac19 100644
--- a/datafusion/proto/src/bytes/registry.rs
+++ b/datafusion/proto/src/bytes/registry.rs
@@ -40,10 +40,6 @@ impl FunctionRegistry for NoRegistry {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Aggregate Function '{name}'")
     }
 
-    fn ordered_set_udaf(&self, name: &str) -> Result<Arc<AggregateUDF>> {
-        plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Ordered Set Aggregate Function '{name}'")
-    }
-
     fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Window Function '{name}'")
     }
@@ -53,12 +49,6 @@ impl FunctionRegistry for NoRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         plan_err!("No function registry provided to deserialize, so can not register User Defined Aggregate Function '{}'", udaf.inner().name())
     }
-    fn register_ordered_set_udaf(
-        &mut self,
-        ordered_set_udaf: Arc<AggregateUDF>,
-    ) -> Result<Option<Arc<AggregateUDF>>> {
-        plan_err!("No function registry provided to deserialize, so can not register User Defined Ordered Set Aggregate Function '{}'", ordered_set_udaf.inner().name())
-    }
     fn register_udf(&mut self, udf: Arc<ScalarUDF>) -> Result<Option<Arc<ScalarUDF>>> {
         plan_err!("No function registry provided to deserialize, so can not deserialize User Defined Function '{}'", udf.inner().name())
     }
diff --git a/datafusion/sql/examples/sql.rs b/datafusion/sql/examples/sql.rs
index 2e64411f041fd..7f1e6bf8f28cc 100644
--- a/datafusion/sql/examples/sql.rs
+++ b/datafusion/sql/examples/sql.rs
@@ -69,7 +69,6 @@ struct MyContextProvider {
     options: ConfigOptions,
     tables: HashMap<String, Arc<dyn TableSource>>,
     udafs: HashMap<String, Arc<AggregateUDF>>,
-    ordered_set_udafs: HashMap<String, Arc<AggregateUDF>>,
     expr_planners: Vec<Arc<dyn ExprPlanner>>,
 }
 
@@ -116,7 +115,6 @@ impl MyContextProvider {
             tables,
             options: Default::default(),
             udafs: Default::default(),
-            ordered_set_udafs: Default::default(),
             expr_planners: vec![],
         }
     }
@@ -144,10 +142,6 @@ impl ContextProvider for MyContextProvider {
         self.udafs.get(name).cloned()
     }
 
-    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
-        self.ordered_set_udafs.get(name).cloned()
-    }
-
     fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
         None
     }
diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs
index f6a7c840a32fa..de753da895d30 100644
--- a/datafusion/sql/src/expr/mod.rs
+++ b/datafusion/sql/src/expr/mod.rs
@@ -1184,13 +1184,6 @@ mod tests {
             }
         }
 
-        fn get_ordered_set_aggregate_meta(
-            &self,
-            _name: &str,
-        ) -> Option<Arc<AggregateUDF>> {
-            None
-        }
-
         fn get_variable_type(&self, _variable_names: &[String]) -> Option<DataType> {
             None
         }
diff --git a/datafusion/sql/tests/common/mod.rs b/datafusion/sql/tests/common/mod.rs
index 5dbc106f97ee7..337fc2ce4f678 100644
--- a/datafusion/sql/tests/common/mod.rs
+++ b/datafusion/sql/tests/common/mod.rs
@@ -54,7 +54,6 @@ impl Display for MockCsvType {
 pub(crate) struct MockSessionState {
     scalar_functions: HashMap<String, Arc<ScalarUDF>>,
     aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
-    ordered_set_aggregate_functions: HashMap<String, Arc<AggregateUDF>>,
     expr_planners: Vec<Arc<dyn ExprPlanner>>,
     type_planner: Option<Arc<dyn TypePlanner>>,
     window_functions: HashMap<String, Arc<WindowUDF>>,
@@ -237,13 +236,6 @@ impl ContextProvider for MockContextProvider {
         self.state.aggregate_functions.get(name).cloned()
     }
 
-    fn get_ordered_set_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>> {
-        self.state
-            .ordered_set_aggregate_functions
-            .get(name)
-            .cloned()
-    }
-
     fn get_variable_type(&self, _: &[String]) -> Option<DataType> {
         unimplemented!()
     }

From 005a27c8a4696647deb24843e62410d09c846753 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 28 Feb 2025 12:26:52 +0900
Subject: [PATCH 22/39] Convert within group to order by when converting sql to
 expr

* Add function to determine it is ordered-set aggregate function
---
 datafusion/expr/src/expr.rs                   |  4 +-
 datafusion/expr/src/udaf.rs                   | 11 ++++
 .../src/approx_percentile_cont.rs             |  4 ++
 .../src/approx_percentile_cont_with_weight.rs |  4 ++
 datafusion/sql/src/expr/function.rs           | 64 +++++++++----------
 datafusion/sql/src/unparser/expr.rs           | 18 +++---
 6 files changed, 64 insertions(+), 41 deletions(-)

diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index fd5186f185458..cabde682ad2fa 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -290,11 +290,13 @@ pub enum Expr {
     /// Represents the call of a scalar function with a set of arguments.
     ScalarFunction(ScalarFunction),
     /// Calls an aggregate function with arguments, and optional
-    /// `ORDER BY`, `FILTER`, `DISTINCT`, `NULL TREATMENT`, and `WITHIN GROUP`.
+    /// `ORDER BY`, `FILTER`, `DISTINCT` and `NULL TREATMENT`.
     ///
     /// See also [`ExprFunctionExt`] to set these fields.
     ///
     /// [`ExprFunctionExt`]: crate::expr_fn::ExprFunctionExt
+    ///
+    /// cf. `WITHIN GROUP` is converted to `ORDER BY` internally in `datafusion/sql/src/expr/function.rs`
     AggregateFunction(AggregateFunction),
     /// Represents the call of a window function with arguments.
     WindowFunction(WindowFunction),
diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index d2aa4f7cca71d..5dcb2fe318f22 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -281,6 +281,11 @@ impl AggregateUDF {
         self.inner.supports_null_handling_clause()
     }
 
+    /// See [`AggregateUDFImpl::is_ordered_set_aggregate`] for more details.
+    pub fn is_ordered_set_aggregate(&self) -> Option<bool> {
+        self.inner.is_ordered_set_aggregate()
+    }
+
     /// Returns the documentation for this Aggregate UDF.
     ///
     /// Documentation can be accessed programmatically as well as
@@ -640,6 +645,12 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
         None
     }
 
+    /// If this function is ordered-set aggregate function, return true
+    /// If the function is not, return false
+    fn is_ordered_set_aggregate(&self) -> Option<bool> {
+        None
+    }
+
     /// Returns the documentation for this Aggregate UDF.
     ///
     /// Documentation can be accessed programmatically as well as
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 5969608cf824d..4cea8e21b54b8 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -325,6 +325,10 @@ impl AggregateUDFImpl for ApproxPercentileCont {
         Some(false)
     }
 
+    fn is_ordered_set_aggregate(&self) -> Option<bool> {
+        Some(true)
+    }
+
     fn documentation(&self) -> Option<&Documentation> {
         self.doc()
     }
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 50281bfcf0b09..07c4fd5b4b835 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
@@ -182,6 +182,10 @@ impl AggregateUDFImpl for ApproxPercentileContWithWeight {
         Some(false)
     }
 
+    fn is_ordered_set_aggregate(&self) -> Option<bool> {
+        Some(true)
+    }
+
     fn documentation(&self) -> Option<&Documentation> {
         self.doc()
     }
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 296e524b7d233..f01308636caaa 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -92,7 +92,7 @@ struct FunctionArgs {
     null_treatment: Option<NullTreatment>,
     /// DISTINCT
     distinct: bool,
-    /// WITHIN_GROUP clause, if any
+    /// WITHIN GROUP clause, if any
     within_group: Vec<OrderByExpr>,
 }
 
@@ -344,12 +344,10 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
-                if let Some(fm) =
-                    self.context_provider.get_ordered_set_aggregate_meta(&name)
+                if fm.is_ordered_set_aggregate().unwrap_or(false)
+                    && within_group.is_empty()
                 {
-                    if within_group.is_empty() {
-                        return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
-                    }
+                    return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
                 }
 
                 if null_treatment.is_some()
@@ -361,35 +359,37 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                     );
                 }
 
-                let order_by = self.order_by_to_sort_expr(
-                    order_by,
-                    schema,
-                    planner_context,
-                    true,
-                    None,
-                )?;
-                let order_by = (!order_by.is_empty()).then_some(order_by);
                 let mut args =
                     self.function_args_to_expr(args, schema, planner_context)?;
 
-                let within_group = self.order_by_to_sort_expr(
-                    within_group,
-                    schema,
-                    planner_context,
-                    false,
-                    None,
-                )?;
-
-                // add target column expression in within group clause to function arguments
-                if !within_group.is_empty() {
-                    args = within_group
-                        .iter()
-                        .map(|sort| sort.expr.clone())
-                        .chain(args)
-                        .collect::<Vec<_>>();
-                }
-
-                let within_group = (!within_group.is_empty()).then_some(within_group);
+                let order_by = if fm.is_ordered_set_aggregate().unwrap_or(false) {
+                    let within_group = self.order_by_to_sort_expr(
+                        within_group,
+                        schema,
+                        planner_context,
+                        false,
+                        None,
+                    )?;
+
+                    // add target column expression in within group clause to function arguments
+                    if !within_group.is_empty() {
+                        args = within_group
+                            .iter()
+                            .map(|sort| sort.expr.clone())
+                            .chain(args)
+                            .collect::<Vec<_>>();
+                    }
+                    (!within_group.is_empty()).then_some(within_group)
+                } else {
+                    let order_by = self.order_by_to_sort_expr(
+                        order_by,
+                        schema,
+                        planner_context,
+                        true,
+                        None,
+                    )?;
+                    (!order_by.is_empty()).then_some(order_by)
+                };
 
                 let filter: Option<Box<Expr>> = filter
                     .map(|e| self.sql_expr_to_logical_expr(*e, schema, planner_context))
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index ffa54aaf5c482..6f6ebc8e1262d 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -283,6 +283,16 @@ impl Unparser<'_> {
             }),
             Expr::AggregateFunction(agg) => {
                 let func_name = agg.func.name();
+                let within_group = if agg.func.is_ordered_set_aggregate() {
+                    agg.order_by
+                        .as_ref()
+                        .unwrap_or(&Vec::new())
+                        .iter()
+                        .map(|sort_expr| self.sort_to_sql(sort_expr))
+                        .collect::<Result<Vec<_>>>()?
+                } else {
+                    Vec::new()
+                };
 
                 let args = self.function_args_to_sql(&agg.args)?;
                 let filter = match &agg.filter {
@@ -290,14 +300,6 @@ impl Unparser<'_> {
                     None => None,
                 };
 
-                let within_group = agg
-                    .within_group
-                    .as_ref()
-                    .unwrap_or(&Vec::new())
-                    .iter()
-                    .map(|sort_expr| self.sort_to_sql(sort_expr))
-                    .collect::<Result<Vec<_>>>()?;
-
                 Ok(ast::Expr::Function(Function {
                     name: ObjectName(vec![Ident {
                         value: func_name.to_string(),

From 1179bc4eb29d85874f88044946a6f280e1820254 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 28 Feb 2025 12:29:05 +0900
Subject: [PATCH 23/39] Rollback within group from proto

---
 datafusion/proto/proto/datafusion.proto  |  1 -
 datafusion/proto/src/generated/pbjson.rs | 18 ------------------
 datafusion/proto/src/generated/prost.rs  |  2 --
 3 files changed, 21 deletions(-)

diff --git a/datafusion/proto/proto/datafusion.proto b/datafusion/proto/proto/datafusion.proto
index 4fd15277630e0..3bc884257daba 100644
--- a/datafusion/proto/proto/datafusion.proto
+++ b/datafusion/proto/proto/datafusion.proto
@@ -516,7 +516,6 @@ message AggregateUDFExprNode {
   LogicalExprNode filter = 3;
   repeated SortExprNode order_by = 4;
   optional bytes fun_definition = 6;
-  repeated SortExprNode within_group = 7;
 }
 
 message ScalarUDFExprNode {
diff --git a/datafusion/proto/src/generated/pbjson.rs b/datafusion/proto/src/generated/pbjson.rs
index ba12ef442d298..add72e4f777e5 100644
--- a/datafusion/proto/src/generated/pbjson.rs
+++ b/datafusion/proto/src/generated/pbjson.rs
@@ -596,9 +596,6 @@ impl serde::Serialize for AggregateUdfExprNode {
         if self.fun_definition.is_some() {
             len += 1;
         }
-        if !self.within_group.is_empty() {
-            len += 1;
-        }
         let mut struct_ser = serializer.serialize_struct("datafusion.AggregateUDFExprNode", len)?;
         if !self.fun_name.is_empty() {
             struct_ser.serialize_field("funName", &self.fun_name)?;
@@ -620,9 +617,6 @@ impl serde::Serialize for AggregateUdfExprNode {
             #[allow(clippy::needless_borrows_for_generic_args)]
             struct_ser.serialize_field("funDefinition", pbjson::private::base64::encode(&v).as_str())?;
         }
-        if !self.within_group.is_empty() {
-            struct_ser.serialize_field("withinGroup", &self.within_group)?;
-        }
         struct_ser.end()
     }
 }
@@ -642,8 +636,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
             "orderBy",
             "fun_definition",
             "funDefinition",
-            "within_group",
-            "withinGroup",
         ];
 
         #[allow(clippy::enum_variant_names)]
@@ -654,7 +646,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
             Filter,
             OrderBy,
             FunDefinition,
-            WithinGroup,
         }
         impl<'de> serde::Deserialize<'de> for GeneratedField {
             fn deserialize<D>(deserializer: D) -> std::result::Result<GeneratedField, D::Error>
@@ -682,7 +673,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                             "filter" => Ok(GeneratedField::Filter),
                             "orderBy" | "order_by" => Ok(GeneratedField::OrderBy),
                             "funDefinition" | "fun_definition" => Ok(GeneratedField::FunDefinition),
-                            "withinGroup" | "within_group" => Ok(GeneratedField::WithinGroup),
                             _ => Err(serde::de::Error::unknown_field(value, FIELDS)),
                         }
                     }
@@ -708,7 +698,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                 let mut filter__ = None;
                 let mut order_by__ = None;
                 let mut fun_definition__ = None;
-                let mut within_group__ = None;
                 while let Some(k) = map_.next_key()? {
                     match k {
                         GeneratedField::FunName => {
@@ -749,12 +738,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                                 map_.next_value::<::std::option::Option<::pbjson::private::BytesDeserialize<_>>>()?.map(|x| x.0)
                             ;
                         }
-                        GeneratedField::WithinGroup => {
-                            if within_group__.is_some() {
-                                return Err(serde::de::Error::duplicate_field("withinGroup"));
-                            }
-                            within_group__ = Some(map_.next_value()?);
-                        }
                     }
                 }
                 Ok(AggregateUdfExprNode {
@@ -764,7 +747,6 @@ impl<'de> serde::Deserialize<'de> for AggregateUdfExprNode {
                     filter: filter__,
                     order_by: order_by__.unwrap_or_default(),
                     fun_definition: fun_definition__,
-                    within_group: within_group__.unwrap_or_default(),
                 })
             }
         }
diff --git a/datafusion/proto/src/generated/prost.rs b/datafusion/proto/src/generated/prost.rs
index 6ee1ee2d50c1c..df32c1a70d618 100644
--- a/datafusion/proto/src/generated/prost.rs
+++ b/datafusion/proto/src/generated/prost.rs
@@ -783,8 +783,6 @@ pub struct AggregateUdfExprNode {
     pub order_by: ::prost::alloc::vec::Vec<SortExprNode>,
     #[prost(bytes = "vec", optional, tag = "6")]
     pub fun_definition: ::core::option::Option<::prost::alloc::vec::Vec<u8>>,
-    #[prost(message, repeated, tag = "7")]
-    pub within_group: ::prost::alloc::vec::Vec<SortExprNode>,
 }
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct ScalarUdfExprNode {

From e5fc1a41d092b9b0db96a16f2749951814805423 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 28 Feb 2025 12:41:03 +0900
Subject: [PATCH 24/39] Utilize within group as order by in functions-aggregate

---
 datafusion/functions-aggregate/src/approx_percentile_cont.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 4cea8e21b54b8..83ebf4e4c9128 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -74,7 +74,7 @@ pub fn approx_percentile_cont(
         args,
         false,
         None,
-        None,
+        within_group,
         None,
     ))
 }

From cf4faad56722c4f77b0a627dbbb8f47959010ab5 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Fri, 28 Feb 2025 13:11:28 +0900
Subject: [PATCH 25/39] Apply clippy

---
 datafusion/optimizer/src/single_distinct_to_groupby.rs | 1 -
 datafusion/sql/src/unparser/expr.rs                    | 2 +-
 datafusion/substrait/src/logical_plan/producer.rs      | 2 ++
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/datafusion/optimizer/src/single_distinct_to_groupby.rs b/datafusion/optimizer/src/single_distinct_to_groupby.rs
index 34f0d1a03d508..c8f3a4bc7859c 100644
--- a/datafusion/optimizer/src/single_distinct_to_groupby.rs
+++ b/datafusion/optimizer/src/single_distinct_to_groupby.rs
@@ -576,7 +576,6 @@ mod tests {
             Some(Box::new(col("a").gt(lit(5)))),
             None,
             None,
-            None,
         ));
         let plan = LogicalPlanBuilder::from(table_scan)
             .aggregate(vec![col("c")], vec![expr, count_distinct(col("b"))])?
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index 6f6ebc8e1262d..0489f442ffa7e 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -283,7 +283,7 @@ impl Unparser<'_> {
             }),
             Expr::AggregateFunction(agg) => {
                 let func_name = agg.func.name();
-                let within_group = if agg.func.is_ordered_set_aggregate() {
+                let within_group = if agg.func.is_ordered_set_aggregate().unwrap_or(false) {
                     agg.order_by
                         .as_ref()
                         .unwrap_or(&Vec::new())
diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs
index 9546a2e1b1968..8b19009d74729 100644
--- a/datafusion/substrait/src/logical_plan/producer.rs
+++ b/datafusion/substrait/src/logical_plan/producer.rs
@@ -1217,6 +1217,8 @@ pub fn from_aggregate_function(
 
     let sorts = if let Some(order_by) = order_by {
         order_by
+            .iter()
+            .map(|expr| to_substrait_sort_field(producer, expr, schema))
             .collect::<Result<Vec<_>>>()?
     } else {
         vec![]

From 5469e39697dd07f59565ab3dce50d8ec1ac05877 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 00:08:30 +0900
Subject: [PATCH 26/39] Convert order by to within group

---
 datafusion/expr/src/udaf.rs         |  8 +++++++-
 datafusion/sql/src/unparser/expr.rs | 11 +++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index 49cf420dd62ba..011e338b26518 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -453,8 +453,14 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
         };
 
         if let Some(order_by) = order_by {
+            let clause = match self.is_ordered_set_aggregate().unwrap_or(false) {
+                true => "WITHIN GROUP",
+                false => "ORDER BY",
+            };
+
             schema_name.write_fmt(format_args!(
-                " ORDER BY [{}]",
+                " {} [{}]",
+                clause,
                 schema_name_from_sorts(order_by)?
             ))?;
         };
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index b211b66157507..3767312330fa6 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -291,6 +291,7 @@ impl Unparser<'_> {
                     distinct,
                     args,
                     filter,
+                    order_by,
                     ..
                 } = &agg.params;
 
@@ -299,6 +300,16 @@ impl Unparser<'_> {
                     Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
                     None => None,
                 };
+                let within_group = if agg.func.is_ordered_set_aggregate().unwrap_or(false) {
+                    order_by
+                        .as_ref()
+                        .unwrap_or(&Vec::new())
+                        .iter()
+                        .map(|sort_expr| self.sort_to_sql(sort_expr))
+                        .collect::<Result<Vec<_>>>()?
+                } else {
+                    Vec::new()
+                };
                 Ok(ast::Expr::Function(Function {
                     name: ObjectName(vec![Ident {
                         value: func_name.to_string(),

From d96b6673f4ef9a623b66da6d9b1f9d6df26d6189 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 00:14:11 +0900
Subject: [PATCH 27/39] Apply cargo fmt

---
 datafusion/expr/src/tree_node.rs                  | 14 +++++++-------
 datafusion/functions-aggregate/src/count.rs       |  1 -
 datafusion/sql/src/unparser/expr.rs               |  3 ++-
 datafusion/substrait/src/logical_plan/consumer.rs |  9 +--------
 4 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs
index 11c3f77a9329c..50af620603469 100644
--- a/datafusion/expr/src/tree_node.rs
+++ b/datafusion/expr/src/tree_node.rs
@@ -257,13 +257,13 @@ impl TreeNode for Expr {
             }) => (args, filter, order_by).map_elements(f)?.map_data(
                 |(new_args, new_filter, new_order_by)| {
                     Ok(Expr::AggregateFunction(AggregateFunction::new_udf(
-                       func,
-                       new_args,
-                       distinct,
-                       new_filter,
-                       new_order_by,
-                       null_treatment,
-                   )))
+                        func,
+                        new_args,
+                        distinct,
+                        new_filter,
+                        new_order_by,
+                        null_treatment,
+                    )))
                 },
             )?,
             Expr::GroupingSet(grouping_set) => match grouping_set {
diff --git a/datafusion/functions-aggregate/src/count.rs b/datafusion/functions-aggregate/src/count.rs
index ff2ba62a9842e..a3339f0fceb94 100644
--- a/datafusion/functions-aggregate/src/count.rs
+++ b/datafusion/functions-aggregate/src/count.rs
@@ -76,7 +76,6 @@ pub fn count_distinct(expr: Expr) -> Expr {
         None,
         None,
         None,
-
     ))
 }
 
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index 3767312330fa6..29990dec09c7f 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -300,7 +300,8 @@ impl Unparser<'_> {
                     Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
                     None => None,
                 };
-                let within_group = if agg.func.is_ordered_set_aggregate().unwrap_or(false) {
+                let within_group = if agg.func.is_ordered_set_aggregate().unwrap_or(false)
+                {
                     order_by
                         .as_ref()
                         .unwrap_or(&Vec::new())
diff --git a/datafusion/substrait/src/logical_plan/consumer.rs b/datafusion/substrait/src/logical_plan/consumer.rs
index 9e5d511db133d..6849053f9a47e 100644
--- a/datafusion/substrait/src/logical_plan/consumer.rs
+++ b/datafusion/substrait/src/logical_plan/consumer.rs
@@ -1946,14 +1946,7 @@ pub async fn from_substrait_agg_func(
     let args = from_substrait_func_args(consumer, &f.arguments, input_schema).await?;
 
     Ok(Arc::new(Expr::AggregateFunction(
-        expr::AggregateFunction::new_udf(
-            udaf,
-            args,
-            distinct,
-            filter,
-            order_by,
-            None,
-        ),
+        expr::AggregateFunction::new_udf(udaf, args, distinct, filter, order_by, None),
     )))
 }
 

From 293d33e48eb3d94cd276c369e0a1e2675fb52422 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 00:21:32 +0900
Subject: [PATCH 28/39] Remove plain line breaks

---
 datafusion/expr/src/registry.rs                   | 1 -
 datafusion/substrait/src/logical_plan/consumer.rs | 1 -
 datafusion/substrait/src/logical_plan/producer.rs | 2 --
 3 files changed, 4 deletions(-)

diff --git a/datafusion/expr/src/registry.rs b/datafusion/expr/src/registry.rs
index 35a002a952bbe..4eb49710bcf85 100644
--- a/datafusion/expr/src/registry.rs
+++ b/datafusion/expr/src/registry.rs
@@ -61,7 +61,6 @@ pub trait FunctionRegistry {
     ) -> Result<Option<Arc<AggregateUDF>>> {
         not_impl_err!("Registering AggregateUDF")
     }
-
     /// Registers a new [`WindowUDF`], returning any previously registered
     /// implementation.
     ///
diff --git a/datafusion/substrait/src/logical_plan/consumer.rs b/datafusion/substrait/src/logical_plan/consumer.rs
index 6849053f9a47e..ffeff3e9df47f 100644
--- a/datafusion/substrait/src/logical_plan/consumer.rs
+++ b/datafusion/substrait/src/logical_plan/consumer.rs
@@ -1220,7 +1220,6 @@ pub async fn from_aggregate_rel(
                         _ if f.invocation == AggregationInvocation::All as i32 => false,
                         _ => false,
                     };
-
                     let order_by = if !f.sorts.is_empty() {
                         Some(
                             from_substrait_sorts(consumer, &f.sorts, input.schema())
diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs
index bd66a4a3fcb75..9dbb246453be5 100644
--- a/datafusion/substrait/src/logical_plan/producer.rs
+++ b/datafusion/substrait/src/logical_plan/producer.rs
@@ -1218,7 +1218,6 @@ pub fn from_aggregate_function(
                 null_treatment: _null_treatment,
             },
     } = agg_fn;
-
     let sorts = if let Some(order_by) = order_by {
         order_by
             .iter()
@@ -1227,7 +1226,6 @@ pub fn from_aggregate_function(
     } else {
         vec![]
     };
-
     let mut arguments: Vec<FunctionArgument> = vec![];
     for arg in args {
         arguments.push(FunctionArgument {

From ecdb21b27988f9526aacc20d0e59add4f2f59de4 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 12:27:18 +0900
Subject: [PATCH 29/39] Remove duplicated column arg in schema name

---
 .../tests/dataframe/dataframe_functions.rs    | 30 +++++++++----------
 datafusion/expr/src/udaf.rs                   |  8 +++++
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 4c88e689bf9fb..817437edc1955 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -364,11 +364,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
 
     let expected = [
-        "+----------------------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |",
-        "+----------------------------------------------------------------------------------+",
-        "| 10                                                                               |",
-        "+----------------------------------------------------------------------------------+",
+        "+---------------------------------------------------------------------------+",
+        "| approx_percentile_cont(Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+---------------------------------------------------------------------------+",
+        "| 10                                                                        |",
+        "+---------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
@@ -386,11 +386,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         approx_percentile_cont(Some(vec![col("b").sort(true, false)]), alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
-        "+---------------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,arg_2) WITHIN GROUP [test.b ASC NULLS LAST] |",
-        "+---------------------------------------------------------------------------+",
-        "| 10                                                                        |",
-        "+---------------------------------------------------------------------------+",
+        "+--------------------------------------------------------------------+",
+        "| approx_percentile_cont(arg_2) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+--------------------------------------------------------------------+",
+        "| 10                                                                 |",
+        "+--------------------------------------------------------------------+",
     ];
     let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
 
@@ -403,11 +403,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         Some(lit(2)),
     );
     let expected = [
-        "+-------------------------------------------------------------------------------------------+",
-        "| approx_percentile_cont(test.b,Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
-        "+-------------------------------------------------------------------------------------------+",
-        "| 30                                                                                        |",
-        "+-------------------------------------------------------------------------------------------+",
+        "+------------------------------------------------------------------------------------+",
+        "| approx_percentile_cont(Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
+        "+------------------------------------------------------------------------------------+",
+        "| 30                                                                                 |",
+        "+------------------------------------------------------------------------------------+",
     ];
 
     let df = create_test_table().await?;
diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index 011e338b26518..7f3b3650129b1 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -435,6 +435,14 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
             null_treatment,
         } = params;
 
+        // exclude the first function argument(= column) in ordered set aggregate function,
+        // because it is duplicated with the WITHIN GROUP clause in schema name.
+        let args = if self.is_ordered_set_aggregate().unwrap_or(false) {
+            &args[1..]
+        } else {
+            &args[..]
+        };
+
         let mut schema_name = String::new();
 
         schema_name.write_fmt(format_args!(

From d65420e48dd8dceb4aa3180f3b68129fae61abfe Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 13:15:19 +0900
Subject: [PATCH 30/39] Refactor boolean functions to just return primitive
 type

---
 datafusion/expr/src/udaf.rs                     | 17 ++++++++---------
 .../src/approx_percentile_cont.rs               |  8 ++++----
 .../src/approx_percentile_cont_with_weight.rs   |  8 ++++----
 datafusion/sql/src/expr/function.rs             |  6 +++---
 datafusion/sql/src/unparser/expr.rs             |  2 +-
 5 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs
index 7f3b3650129b1..574fcb878ab0f 100644
--- a/datafusion/expr/src/udaf.rs
+++ b/datafusion/expr/src/udaf.rs
@@ -309,12 +309,12 @@ impl AggregateUDF {
     }
 
     /// See [`AggregateUDFImpl::supports_null_handling_clause`] for more details.
-    pub fn supports_null_handling_clause(&self) -> Option<bool> {
+    pub fn supports_null_handling_clause(&self) -> bool {
         self.inner.supports_null_handling_clause()
     }
 
     /// See [`AggregateUDFImpl::is_ordered_set_aggregate`] for more details.
-    pub fn is_ordered_set_aggregate(&self) -> Option<bool> {
+    pub fn is_ordered_set_aggregate(&self) -> bool {
         self.inner.is_ordered_set_aggregate()
     }
 
@@ -437,7 +437,7 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
 
         // exclude the first function argument(= column) in ordered set aggregate function,
         // because it is duplicated with the WITHIN GROUP clause in schema name.
-        let args = if self.is_ordered_set_aggregate().unwrap_or(false) {
+        let args = if self.is_ordered_set_aggregate() {
             &args[1..]
         } else {
             &args[..]
@@ -461,7 +461,7 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
         };
 
         if let Some(order_by) = order_by {
-            let clause = match self.is_ordered_set_aggregate().unwrap_or(false) {
+            let clause = match self.is_ordered_set_aggregate() {
                 true => "WITHIN GROUP",
                 false => "ORDER BY",
             };
@@ -871,15 +871,14 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
 
     /// If this function supports `[IGNORE NULLS | RESPECT NULLS]` clause, return true
     /// If the function does not, return false
-    /// Otherwise return None (the default)
-    fn supports_null_handling_clause(&self) -> Option<bool> {
-        None
+    fn supports_null_handling_clause(&self) -> bool {
+        true
     }
 
     /// If this function is ordered-set aggregate function, return true
     /// If the function is not, return false
-    fn is_ordered_set_aggregate(&self) -> Option<bool> {
-        None
+    fn is_ordered_set_aggregate(&self) -> bool {
+        false
     }
 
     /// Returns the documentation for this Aggregate UDF.
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 0519ca8946eb8..e3a9b43108827 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -320,12 +320,12 @@ impl AggregateUDFImpl for ApproxPercentileCont {
         Ok(arg_types[0].clone())
     }
 
-    fn supports_null_handling_clause(&self) -> Option<bool> {
-        Some(false)
+    fn supports_null_handling_clause(&self) -> bool {
+        false
     }
 
-    fn is_ordered_set_aggregate(&self) -> Option<bool> {
-        Some(true)
+    fn is_ordered_set_aggregate(&self) -> bool {
+        true
     }
 
     fn documentation(&self) -> Option<&Documentation> {
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 07c4fd5b4b835..0316757f26d08 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs
@@ -178,12 +178,12 @@ impl AggregateUDFImpl for ApproxPercentileContWithWeight {
         self.approx_percentile_cont.state_fields(args)
     }
 
-    fn supports_null_handling_clause(&self) -> Option<bool> {
-        Some(false)
+    fn supports_null_handling_clause(&self) -> bool {
+        false
     }
 
-    fn is_ordered_set_aggregate(&self) -> Option<bool> {
-        Some(true)
+    fn is_ordered_set_aggregate(&self) -> bool {
+        true
     }
 
     fn documentation(&self) -> Option<&Documentation> {
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 363eb072834c1..faa779bd9f1e8 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -365,14 +365,14 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
-                if fm.is_ordered_set_aggregate().unwrap_or(false)
+                if fm.is_ordered_set_aggregate()
                     && within_group.is_empty()
                 {
                     return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
                 }
 
                 if null_treatment.is_some()
-                    && !fm.supports_null_handling_clause().unwrap_or(true)
+                    && !fm.supports_null_handling_clause()
                 {
                     return plan_err!(
                         "[IGNORE | RESPECT] NULLS are not permitted for {}",
@@ -383,7 +383,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 let mut args =
                     self.function_args_to_expr(args, schema, planner_context)?;
 
-                let order_by = if fm.is_ordered_set_aggregate().unwrap_or(false) {
+                let order_by = if fm.is_ordered_set_aggregate() {
                     let within_group = self.order_by_to_sort_expr(
                         within_group,
                         schema,
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index 29990dec09c7f..aa402380ae69b 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -300,7 +300,7 @@ impl Unparser<'_> {
                     Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
                     None => None,
                 };
-                let within_group = if agg.func.is_ordered_set_aggregate().unwrap_or(false)
+                let within_group = if agg.func.is_ordered_set_aggregate()
                 {
                     order_by
                         .as_ref()

From b6d426ae25d84a03006b15148fcce5cdab6532af Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 13:28:48 +0900
Subject: [PATCH 31/39] Make within group necessary in the signature of
 existing ordered set aggr funcs

---
 datafusion/core/tests/dataframe/dataframe_functions.rs     | 6 +++---
 .../functions-aggregate/src/approx_percentile_cont.rs      | 7 +++----
 datafusion/proto/tests/cases/roundtrip_logical_plan.rs     | 4 ++--
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 817437edc1955..133e0445316fc 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -361,7 +361,7 @@ async fn test_fn_approx_median() -> Result<()> {
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
     let expr =
-        approx_percentile_cont(Some(vec![col("b").sort(true, false)]), lit(0.5), None);
+        approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
 
     let expected = [
         "+---------------------------------------------------------------------------+",
@@ -383,7 +383,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         "arg_2".to_string(),
     ));
     let expr =
-        approx_percentile_cont(Some(vec![col("b").sort(true, false)]), alias_expr, None);
+        approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
         "+--------------------------------------------------------------------+",
@@ -398,7 +398,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     // with number of centroids set
     let expr = approx_percentile_cont(
-        Some(vec![col("b").sort(true, false)]),
+        vec![col("b").sort(true, false)],
         lit(0.5),
         Some(lit(2)),
     );
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index e3a9b43108827..36639298f9bea 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -52,13 +52,12 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
 
 /// Computes the approximate percentile continuous of a set of numbers
 pub fn approx_percentile_cont(
-    within_group: Option<Vec<Sort>>,
+    within_group: Vec<Sort>,
     percentile: Expr,
     centroids: Option<Expr>,
 ) -> Expr {
     let expr = within_group
-        .as_ref()
-        .and_then(|v| v.first())
+        .first()
         .map(|sort| sort.expr.clone())
         .unwrap_or_default();
 
@@ -73,7 +72,7 @@ pub fn approx_percentile_cont(
         args,
         false,
         None,
-        within_group,
+        Some(within_group),
         None,
     ))
 }
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index b45d71407f87e..5a543b85f8c82 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -970,9 +970,9 @@ async fn roundtrip_expr_api() -> Result<()> {
         stddev_pop(lit(2.2)),
         approx_distinct(lit(2)),
         approx_median(lit(2)),
-        approx_percentile_cont(Some(vec![lit(2).sort(true, false)]), lit(0.5), None),
+        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), None),
         approx_percentile_cont(
-            Some(vec![lit(2).sort(true, false)]),
+            vec![lit(2).sort(true, false)],
             lit(0.5),
             Some(lit(50)),
         ),

From 4b0c52fd66eeb3ab851869f91b2ec87977a408da Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 13:34:32 +0900
Subject: [PATCH 32/39] Apply cargo fmt

---
 .../core/tests/dataframe/dataframe_functions.rs     | 13 ++++---------
 .../proto/tests/cases/roundtrip_logical_plan.rs     |  6 +-----
 datafusion/sql/src/expr/function.rs                 |  8 ++------
 datafusion/sql/src/unparser/expr.rs                 |  3 +--
 4 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 133e0445316fc..0bb19c450d40b 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -360,8 +360,7 @@ async fn test_fn_approx_median() -> Result<()> {
 
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
-    let expr =
-        approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
+    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
 
     let expected = [
         "+---------------------------------------------------------------------------+",
@@ -382,8 +381,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         None::<&str>,
         "arg_2".to_string(),
     ));
-    let expr =
-        approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
+    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
         "+--------------------------------------------------------------------+",
@@ -397,11 +395,8 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_batches_eq!(expected, &batches);
 
     // with number of centroids set
-    let expr = approx_percentile_cont(
-        vec![col("b").sort(true, false)],
-        lit(0.5),
-        Some(lit(2)),
-    );
+    let expr =
+        approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), Some(lit(2)));
     let expected = [
         "+------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index 5a543b85f8c82..86a58fbc1468a 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -971,11 +971,7 @@ async fn roundtrip_expr_api() -> Result<()> {
         approx_distinct(lit(2)),
         approx_median(lit(2)),
         approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), None),
-        approx_percentile_cont(
-            vec![lit(2).sort(true, false)],
-            lit(0.5),
-            Some(lit(50)),
-        ),
+        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), Some(lit(50))),
         approx_percentile_cont_with_weight(lit(2), lit(1), lit(0.5)),
         grouping(lit(1)),
         bit_and(lit(2)),
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index faa779bd9f1e8..57a3f0e17c774 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -365,15 +365,11 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         } else {
             // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function
             if let Some(fm) = self.context_provider.get_aggregate_meta(&name) {
-                if fm.is_ordered_set_aggregate()
-                    && within_group.is_empty()
-                {
+                if fm.is_ordered_set_aggregate() && within_group.is_empty() {
                     return plan_err!("WITHIN GROUP clause is required when calling ordered set aggregate function({})", fm.name());
                 }
 
-                if null_treatment.is_some()
-                    && !fm.supports_null_handling_clause()
-                {
+                if null_treatment.is_some() && !fm.supports_null_handling_clause() {
                     return plan_err!(
                         "[IGNORE | RESPECT] NULLS are not permitted for {}",
                         fm.name()
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index aa402380ae69b..99c765eb8a9d1 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -300,8 +300,7 @@ impl Unparser<'_> {
                     Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
                     None => None,
                 };
-                let within_group = if agg.func.is_ordered_set_aggregate()
-                {
+                let within_group = if agg.func.is_ordered_set_aggregate() {
                     order_by
                         .as_ref()
                         .unwrap_or(&Vec::new())

From 36a732d6258ec13676d2153e218d18c4efbf00b9 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 14:03:00 +0900
Subject: [PATCH 33/39] Support a single ordering expression in the signature

---
 datafusion/core/tests/dataframe/dataframe_functions.rs   | 6 +++---
 .../functions-aggregate/src/approx_percentile_cont.rs    | 9 +++------
 datafusion/proto/tests/cases/roundtrip_logical_plan.rs   | 4 ++--
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 0bb19c450d40b..acd6fcf76f57b 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -360,7 +360,7 @@ async fn test_fn_approx_median() -> Result<()> {
 
 #[tokio::test]
 async fn test_fn_approx_percentile_cont() -> Result<()> {
-    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), None);
+    let expr = approx_percentile_cont(col("b").sort(true, false), lit(0.5), None);
 
     let expected = [
         "+---------------------------------------------------------------------------+",
@@ -381,7 +381,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
         None::<&str>,
         "arg_2".to_string(),
     ));
-    let expr = approx_percentile_cont(vec![col("b").sort(true, false)], alias_expr, None);
+    let expr = approx_percentile_cont(col("b").sort(true, false), alias_expr, None);
     let df = create_test_table().await?;
     let expected = [
         "+--------------------------------------------------------------------+",
@@ -396,7 +396,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     // with number of centroids set
     let expr =
-        approx_percentile_cont(vec![col("b").sort(true, false)], lit(0.5), Some(lit(2)));
+        approx_percentile_cont(col("b").sort(true, false), lit(0.5), Some(lit(2)));
     let expected = [
         "+------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 36639298f9bea..7c4d11c927475 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -52,14 +52,11 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
 
 /// Computes the approximate percentile continuous of a set of numbers
 pub fn approx_percentile_cont(
-    within_group: Vec<Sort>,
+    within_group: Sort,
     percentile: Expr,
     centroids: Option<Expr>,
 ) -> Expr {
-    let expr = within_group
-        .first()
-        .map(|sort| sort.expr.clone())
-        .unwrap_or_default();
+    let expr = within_group.expr.clone();
 
     let args = if let Some(centroids) = centroids {
         vec![expr, percentile, centroids]
@@ -72,7 +69,7 @@ pub fn approx_percentile_cont(
         args,
         false,
         None,
-        Some(within_group),
+        Some(vec![within_group]),
         None,
     ))
 }
diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
index 86a58fbc1468a..7fe586d64e6b2 100644
--- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
+++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
@@ -970,8 +970,8 @@ async fn roundtrip_expr_api() -> Result<()> {
         stddev_pop(lit(2.2)),
         approx_distinct(lit(2)),
         approx_median(lit(2)),
-        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), None),
-        approx_percentile_cont(vec![lit(2).sort(true, false)], lit(0.5), Some(lit(50))),
+        approx_percentile_cont(lit(2).sort(true, false), lit(0.5), None),
+        approx_percentile_cont(lit(2).sort(true, false), lit(0.5), Some(lit(50))),
         approx_percentile_cont_with_weight(lit(2), lit(1), lit(0.5)),
         grouping(lit(1)),
         bit_and(lit(2)),

From 8d6db85d5d54c59800c5f231b94f4e13266fe5cd Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 14:04:51 +0900
Subject: [PATCH 34/39] Apply cargo fmt

---
 datafusion/core/tests/dataframe/dataframe_functions.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index acd6fcf76f57b..1a26d56deee14 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -395,8 +395,7 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_batches_eq!(expected, &batches);
 
     // with number of centroids set
-    let expr =
-        approx_percentile_cont(col("b").sort(true, false), lit(0.5), Some(lit(2)));
+    let expr = approx_percentile_cont(col("b").sort(true, false), lit(0.5), Some(lit(2)));
     let expected = [
         "+------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |",

From db0355a532aa2aae4d45eebe05264a80fb2fa04a Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 22:04:53 +0900
Subject: [PATCH 35/39] Add dataframe function test cases to verify descending
 ordering

---
 .../tests/dataframe/dataframe_functions.rs    | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 1a26d56deee14..1aec6c23b18b0 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -375,6 +375,21 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     assert_batches_eq!(expected, &batches);
 
+    let expr = approx_percentile_cont(col("b").sort(false, false), lit(0.1), None);
+
+    let expected = [
+        "+----------------------------------------------------------------------------+",
+        "| approx_percentile_cont(Float64(0.1)) WITHIN GROUP [test.b DESC NULLS LAST] |",
+        "+----------------------------------------------------------------------------+",
+        "| 100                                                                        |",
+        "+----------------------------------------------------------------------------+",
+    ];
+
+    let df = create_test_table().await?;
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_batches_eq!(expected, &batches);
+
     // the arg2 parameter is a complex expr, but it can be evaluated to the literal value
     let alias_expr = Expr::Alias(Alias::new(
         cast(lit(0.5), DataType::Float32),
@@ -394,6 +409,24 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     assert_batches_eq!(expected, &batches);
 
+    let alias_expr = Expr::Alias(Alias::new(
+        cast(lit(0.1), DataType::Float32),
+        None::<&str>,
+        "arg_2".to_string(),
+    ));
+    let expr = approx_percentile_cont(col("b").sort(false, false), alias_expr, None);
+    let df = create_test_table().await?;
+    let expected = [
+        "+---------------------------------------------------------------------+",
+        "| approx_percentile_cont(arg_2) WITHIN GROUP [test.b DESC NULLS LAST] |",
+        "+---------------------------------------------------------------------+",
+        "| 100                                                                 |",
+        "+---------------------------------------------------------------------+",
+    ];
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_batches_eq!(expected, &batches);
+
     // with number of centroids set
     let expr = approx_percentile_cont(col("b").sort(true, false), lit(0.5), Some(lit(2)));
     let expected = [
@@ -409,6 +442,20 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     assert_batches_eq!(expected, &batches);
 
+    let expr = approx_percentile_cont(col("b").sort(false, false), lit(0.1), Some(lit(2)));
+    let expected = [
+        "+-------------------------------------------------------------------------------------+",
+        "| approx_percentile_cont(Float64(0.1),Int32(2)) WITHIN GROUP [test.b DESC NULLS LAST] |",
+        "+-------------------------------------------------------------------------------------+",
+        "| 69                                                                                  |",
+        "+-------------------------------------------------------------------------------------+",
+    ];
+
+    let df = create_test_table().await?;
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_batches_eq!(expected, &batches);
+
     Ok(())
 }
 

From 37b783e7a87137cabffffd34794e2554cfad4a21 Mon Sep 17 00:00:00 2001
From: Garam Choi <Starever222@gmail.com>
Date: Sat, 1 Mar 2025 22:07:31 +0900
Subject: [PATCH 36/39] Apply cargo fmt

---
 datafusion/core/tests/dataframe/dataframe_functions.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 1aec6c23b18b0..8ada55d5caa0d 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -442,7 +442,8 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
 
     assert_batches_eq!(expected, &batches);
 
-    let expr = approx_percentile_cont(col("b").sort(false, false), lit(0.1), Some(lit(2)));
+    let expr =
+        approx_percentile_cont(col("b").sort(false, false), lit(0.1), Some(lit(2)));
     let expected = [
         "+-------------------------------------------------------------------------------------+",
         "| approx_percentile_cont(Float64(0.1),Int32(2)) WITHIN GROUP [test.b DESC NULLS LAST] |",

From 124d8c58d55961ded26d1258b8d692ec7d047444 Mon Sep 17 00:00:00 2001
From: garamda <Starever222@gmail.com>
Date: Wed, 5 Mar 2025 14:56:42 +0900
Subject: [PATCH 37/39] Apply code reviews

* Uses order by consistently after done with sql

* Remove redundant comment

* Serve more clear error msg

* Handle error cases in the same code block
---
 datafusion/expr/src/expr.rs                              | 2 --
 .../functions-aggregate/src/approx_percentile_cont.rs    | 6 +++---
 datafusion/sql/src/expr/function.rs                      | 9 ++++-----
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs
index 6b46948632551..f8baf9c94b3cf 100644
--- a/datafusion/expr/src/expr.rs
+++ b/datafusion/expr/src/expr.rs
@@ -295,8 +295,6 @@ pub enum Expr {
     /// See also [`ExprFunctionExt`] to set these fields.
     ///
     /// [`ExprFunctionExt`]: crate::expr_fn::ExprFunctionExt
-    ///
-    /// cf. `WITHIN GROUP` is converted to `ORDER BY` internally in `datafusion/sql/src/expr/function.rs`
     AggregateFunction(AggregateFunction),
     /// Represents the call of a window function with arguments.
     WindowFunction(WindowFunction),
diff --git a/datafusion/functions-aggregate/src/approx_percentile_cont.rs b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
index 7c4d11c927475..41281733f5deb 100644
--- a/datafusion/functions-aggregate/src/approx_percentile_cont.rs
+++ b/datafusion/functions-aggregate/src/approx_percentile_cont.rs
@@ -52,11 +52,11 @@ create_func!(ApproxPercentileCont, approx_percentile_cont_udaf);
 
 /// Computes the approximate percentile continuous of a set of numbers
 pub fn approx_percentile_cont(
-    within_group: Sort,
+    order_by: Sort,
     percentile: Expr,
     centroids: Option<Expr>,
 ) -> Expr {
-    let expr = within_group.expr.clone();
+    let expr = order_by.expr.clone();
 
     let args = if let Some(centroids) = centroids {
         vec![expr, percentile, centroids]
@@ -69,7 +69,7 @@ pub fn approx_percentile_cont(
         args,
         false,
         None,
-        Some(vec![within_group]),
+        Some(vec![order_by]),
         None,
     ))
 }
diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs
index 57a3f0e17c774..e8ccca542dfc8 100644
--- a/datafusion/sql/src/expr/function.rs
+++ b/datafusion/sql/src/expr/function.rs
@@ -148,6 +148,9 @@ impl FunctionArgs {
                 }
                 FunctionArgumentClause::OrderBy(oby) => {
                     if order_by.is_some() {
+                        if !within_group.is_empty() {
+                            return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used");
+                        }
                         return not_impl_err!("Calling {name}: Duplicated ORDER BY clause in function arguments");
                     }
                     order_by = Some(oby);
@@ -180,13 +183,9 @@ impl FunctionArgs {
             }
         }
 
-        if !within_group.is_empty() && order_by.is_some() {
-            return plan_err!("ORDER BY clause is only permitted in WITHIN GROUP clause when a WITHIN GROUP is used");
-        }
-
         if within_group.len() > 1 {
             return not_impl_err!(
-                "Multiple column ordering in WITHIN GROUP clause is not supported"
+                "Only a single ordering expression is permitted in a WITHIN GROUP clause"
             );
         }
 

From 3259c95dc78d4a463c2fa3a0dbfb382156c0b5f3 Mon Sep 17 00:00:00 2001
From: garamda <Starever222@gmail.com>
Date: Wed, 5 Mar 2025 16:37:30 +0900
Subject: [PATCH 38/39] Update error msg in test as corresponding code changed

---
 datafusion/sqllogictest/test_files/aggregate.slt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt
index 2189ed2ddf731..61a34eca6d1a0 100644
--- a/datafusion/sqllogictest/test_files/aggregate.slt
+++ b/datafusion/sqllogictest/test_files/aggregate.slt
@@ -169,7 +169,7 @@ SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) IGNORE NULLS FROM
 statement error DataFusion error: Error during planning: \[IGNORE | RESPECT\] NULLS are not permitted for approx_percentile_cont
 SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5) RESPECT NULLS FROM aggregate_test_100
 
-statement error DataFusion error: This feature is not implemented: Multiple column ordering in WITHIN GROUP clause is not supported
+statement error DataFusion error: This feature is not implemented: Only a single ordering expression is permitted in a WITHIN GROUP clause
 SELECT approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c5, c12) FROM aggregate_test_100
 
 # Not supported over sliding windows

From 40d705519f5bfd8062191c8c2273b5d0b74344d5 Mon Sep 17 00:00:00 2001
From: Jay Zhan <jayzhan211@gmail.com>
Date: Wed, 23 Apr 2025 17:27:23 +0800
Subject: [PATCH 39/39] fix

---
 .../core/benches/aggregate_query_sql.rs       |  2 +
 .../tests/dataframe/dataframe_functions.rs    | 83 +++++++++++++++----
 2 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/datafusion/core/benches/aggregate_query_sql.rs b/datafusion/core/benches/aggregate_query_sql.rs
index 88ca69fb32427..057a0e1d1b54c 100644
--- a/datafusion/core/benches/aggregate_query_sql.rs
+++ b/datafusion/core/benches/aggregate_query_sql.rs
@@ -157,6 +157,7 @@ fn criterion_benchmark(c: &mut Criterion) {
         b.iter(|| {
             query(
                 ctx.clone(),
+                &rt,
                 "SELECT utf8, approx_percentile_cont(0.5, 2500) WITHIN GROUP (ORDER BY u64_wide)  \
                  FROM t GROUP BY utf8",
             )
@@ -167,6 +168,7 @@ fn criterion_benchmark(c: &mut Criterion) {
         b.iter(|| {
             query(
                 ctx.clone(),
+                &rt,
                 "SELECT utf8, approx_percentile_cont(0.5, 2500) WITHIN GROUP (ORDER BY f32)  \
                  FROM t GROUP BY utf8",
             )
diff --git a/datafusion/core/tests/dataframe/dataframe_functions.rs b/datafusion/core/tests/dataframe/dataframe_functions.rs
index 3f95bfe79057e..40590d74ad910 100644
--- a/datafusion/core/tests/dataframe/dataframe_functions.rs
+++ b/datafusion/core/tests/dataframe/dataframe_functions.rs
@@ -392,11 +392,26 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_snapshot!(
         batches_to_string(&batches),
         @r"
-    +---------------------------------------------+
-    | approx_percentile_cont(test.b,Float64(0.5)) |
-    +---------------------------------------------+
-    | 10                                          |
-    +---------------------------------------------+
+    +---------------------------------------------------------------------------+
+    | approx_percentile_cont(Float64(0.5)) WITHIN GROUP [test.b ASC NULLS LAST] |
+    +---------------------------------------------------------------------------+
+    | 10                                                                        |
+    +---------------------------------------------------------------------------+
+    ");
+
+    let expr = approx_percentile_cont(col("b").sort(false, false), lit(0.1), None);
+
+    let df = create_test_table().await?;
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_snapshot!(
+        batches_to_string(&batches),
+        @r"
+    +----------------------------------------------------------------------------+
+    | approx_percentile_cont(Float64(0.1)) WITHIN GROUP [test.b DESC NULLS LAST] |
+    +----------------------------------------------------------------------------+
+    | 100                                                                        |
+    +----------------------------------------------------------------------------+
     ");
 
     // the arg2 parameter is a complex expr, but it can be evaluated to the literal value
@@ -412,16 +427,52 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_snapshot!(
         batches_to_string(&batches),
         @r"
-    +--------------------------------------+
-    | approx_percentile_cont(test.b,arg_2) |
-    +--------------------------------------+
-    | 10                                   |
-    +--------------------------------------+
+    +--------------------------------------------------------------------+
+    | approx_percentile_cont(arg_2) WITHIN GROUP [test.b ASC NULLS LAST] |
+    +--------------------------------------------------------------------+
+    | 10                                                                 |
+    +--------------------------------------------------------------------+
+    "
+    );
+
+    let alias_expr = Expr::Alias(Alias::new(
+        cast(lit(0.1), DataType::Float32),
+        None::<&str>,
+        "arg_2".to_string(),
+    ));
+    let expr = approx_percentile_cont(col("b").sort(false, false), alias_expr, None);
+    let df = create_test_table().await?;
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_snapshot!(
+        batches_to_string(&batches),
+        @r"
+    +---------------------------------------------------------------------+
+    | approx_percentile_cont(arg_2) WITHIN GROUP [test.b DESC NULLS LAST] |
+    +---------------------------------------------------------------------+
+    | 100                                                                 |
+    +---------------------------------------------------------------------+
     "
     );
 
     // with number of centroids set
-    let expr = approx_percentile_cont(col("b"), lit(0.5), Some(lit(2)));
+    let expr = approx_percentile_cont(col("b").sort(true, false), lit(0.5), Some(lit(2)));
+
+    let df = create_test_table().await?;
+    let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
+
+    assert_snapshot!(
+        batches_to_string(&batches),
+        @r"
+    +------------------------------------------------------------------------------------+
+    | approx_percentile_cont(Float64(0.5),Int32(2)) WITHIN GROUP [test.b ASC NULLS LAST] |
+    +------------------------------------------------------------------------------------+
+    | 30                                                                                 |
+    +------------------------------------------------------------------------------------+
+    ");
+
+    let expr =
+        approx_percentile_cont(col("b").sort(false, false), lit(0.1), Some(lit(2)));
 
     let df = create_test_table().await?;
     let batches = df.aggregate(vec![], vec![expr]).unwrap().collect().await?;
@@ -429,11 +480,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
     assert_snapshot!(
         batches_to_string(&batches),
         @r"
-    +------------------------------------------------------+
-    | approx_percentile_cont(test.b,Float64(0.5),Int32(2)) |
-    +------------------------------------------------------+
-    | 30                                                   |
-    +------------------------------------------------------+
+    +-------------------------------------------------------------------------------------+
+    | approx_percentile_cont(Float64(0.1),Int32(2)) WITHIN GROUP [test.b DESC NULLS LAST] |
+    +-------------------------------------------------------------------------------------+
+    | 69                                                                                  |
+    +-------------------------------------------------------------------------------------+
     ");
 
     Ok(())
