From 799afcbf0b0f9ea0aaf0e2a372d713ce0f405606 Mon Sep 17 00:00:00 2001
From: Jacob Sherin <jacob@protoship.io>
Date: Thu, 25 Jul 2024 23:45:29 +0530
Subject: [PATCH 1/2] Refactor: use `AccumulatorArgs::is_reversed`

---
 .../functions-aggregate/src/nth_value.rs       | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/datafusion/functions-aggregate/src/nth_value.rs b/datafusion/functions-aggregate/src/nth_value.rs
index 9bbd68c9bdf60..fb13599a30b79 100644
--- a/datafusion/functions-aggregate/src/nth_value.rs
+++ b/datafusion/functions-aggregate/src/nth_value.rs
@@ -30,8 +30,7 @@ use datafusion_common::{exec_err, internal_err, not_impl_err, Result, ScalarValu
 use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
 use datafusion_expr::utils::format_state_name;
 use datafusion_expr::{
-    Accumulator, AggregateUDF, AggregateUDFImpl, Expr, ReversedUDAF, Signature,
-    Volatility,
+    Accumulator, AggregateUDFImpl, Expr, ReversedUDAF, Signature, Volatility,
 };
 use datafusion_physical_expr_common::aggregate::merge_arrays::merge_ordered_arrays;
 use datafusion_physical_expr_common::aggregate::utils::ordering_fields;
@@ -53,9 +52,6 @@ make_udaf_expr_and_func!(
 #[derive(Debug)]
 pub struct NthValueAgg {
     signature: Signature,
-    /// Determines whether `N` is relative to the beginning or the end
-    /// of the aggregation. When set to `true`, then `N` is from the end.
-    reversed: bool,
 }
 
 impl NthValueAgg {
@@ -63,14 +59,8 @@ impl NthValueAgg {
     pub fn new() -> Self {
         Self {
             signature: Signature::any(2, Volatility::Immutable),
-            reversed: false,
         }
     }
-
-    pub fn with_reversed(mut self, reversed: bool) -> Self {
-        self.reversed = reversed;
-        self
-    }
 }
 
 impl Default for NthValueAgg {
@@ -99,7 +89,7 @@ impl AggregateUDFImpl for NthValueAgg {
     fn accumulator(&self, acc_args: AccumulatorArgs) -> Result<Box<dyn Accumulator>> {
         let n = match acc_args.input_exprs[1] {
             Expr::Literal(ScalarValue::Int64(Some(value))) => {
-                if self.reversed {
+                if acc_args.is_reversed {
                     Ok(-value)
                 } else {
                     Ok(value)
@@ -157,9 +147,7 @@ impl AggregateUDFImpl for NthValueAgg {
     }
 
     fn reverse_expr(&self) -> ReversedUDAF {
-        ReversedUDAF::Reversed(Arc::from(AggregateUDF::from(
-            Self::new().with_reversed(!self.reversed),
-        )))
+        ReversedUDAF::Reversed(nth_value_udaf())
     }
 }
 

From f817d97fff8c035b92f072d4c8fd20aee45c78f8 Mon Sep 17 00:00:00 2001
From: Jacob Sherin <jacob@protoship.io>
Date: Thu, 25 Jul 2024 23:59:23 +0530
Subject: [PATCH 2/2] Minor: fixes comment

---
 datafusion/physical-expr-common/src/aggregate/mod.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datafusion/physical-expr-common/src/aggregate/mod.rs b/datafusion/physical-expr-common/src/aggregate/mod.rs
index b58a5a6faf242..4eede6567504a 100644
--- a/datafusion/physical-expr-common/src/aggregate/mod.rs
+++ b/datafusion/physical-expr-common/src/aggregate/mod.rs
@@ -485,7 +485,7 @@ impl AggregateFunctionExpr {
         self.ignore_nulls
     }
 
-    /// Return if the aggregation is distinct
+    /// Return if the aggregation is reversed
     pub fn is_reversed(&self) -> bool {
         self.is_reversed
     }