Introduce Sum UDAF #10651

jayzhan211 · 2024-05-24T13:01:24Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Add AccumulatorArgs to create_groups_accumulator

This PR only introduce Sum UDAF, remove builtin is not included to keep the PR small.

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <[email protected]>

alamb

Looks great to me @jayzhan211 -- thank you. I left some small comments, but overall I think it looks really nice.

alamb · 2024-05-30T20:32:46Z

datafusion/core/tests/user_defined/user_defined_aggregates.rs

@@ -142,7 +142,7 @@ async fn test_udaf_as_window_with_frame_without_retract_batch() {
    let sql = "SELECT time_sum(time) OVER(ORDER BY time ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING) as time_sum from t";
    // Note if this query ever does start working
    let err = execute(&ctx, sql).await.unwrap_err();
-    assert_contains!(err.to_string(), "This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented: AggregateUDF { inner: AggregateUDF { name: \"time_sum\", signature: Signature { type_signature: Exact([Timestamp(Nanosecond, None)]), volatility: Immutable }, fun: \"<FUNC>\" } }(t.time) ORDER BY [t.time ASC NULLS LAST] ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING");
+    assert_contains!(err.to_string(), "This feature is not implemented: Aggregate can not be used as a sliding accumulator because `retract_batch` is not implemented: time_sum(t.time) ORDER BY [t.time ASC NULLS LAST] ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING");


that is certainly nicer

alamb · 2024-05-30T20:34:04Z

datafusion/expr/src/udaf.rs

+
+    /// Coerce arguments of a function call to types that the function can evaluate.
+    ///
+    /// This function is only called if [`AggregateUDFImpl::signature`] returns [`crate::TypeSignature::UserDefined`]. Most


alamb · 2024-05-30T20:37:09Z

datafusion/expr/src/udaf.rs

@@ -389,6 +410,13 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
        &[]
    }

+    fn create_sliding_accumulator(


Could we please add some documentation here explaining what this is?

Basically I think this is to allow returning a different Accumulator instance that is optimized for sliding windows (e.g. incrementally computing output via https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.Accumulator.html#method.retract_batch

alamb · 2024-05-30T20:37:21Z

datafusion/expr/src/udaf.rs

@@ -459,7 +515,7 @@ pub enum ReversedUDAF {
    /// The expression does not support reverse calculation, like ArrayAgg
    NotSupported,
    /// The expression is different from the original expression
-    Reversed(Arc<dyn AggregateUDFImpl>),
+    Reversed(Arc<AggregateUDF>),


datafusion/functions-aggregate/src/lib.rs

Signed-off-by: jayzhan211 <[email protected]>

mustafasrepo · 2024-05-31T07:10:00Z

datafusion/expr/src/expr.rs

+            if fun.to_string() == "first_value" || fun.to_string() == "last_value" {
+                assert_eq!(fun.to_string(), name);
+            } else {
+                assert_eq!(fun.to_string(), name.to_uppercase());


I wonder, maybe we should treat udf names case insensitive way. What do you think?

#10695 track the issue to rename name to lowercase

mustafasrepo · 2024-05-31T07:13:14Z

datafusion/expr/src/expr_schema.rs

+                        let data_types = args
+                            .iter()
+                            .map(|e| e.get_type(schema))
+                            .collect::<Result<Vec<_>>>()?;


Do we need this section?. It seems that data_types is already calculated at the outer scope.

mustafasrepo · 2024-05-31T07:17:09Z

datafusion/expr/src/expr_schema.rs

+                    WindowFunctionDefinition::AggregateUDF(udf) => {
+                        let new_types = data_types_with_aggregate_udf(&data_types, udf).map_err(|err| {
+                            plan_datafusion_err!(
+                                "{} and {}",
+                                err,
+                                utils::generate_signature_error_msg(
+                                    fun.name(),
+                                    fun.signature().clone(),
+                                    &data_types
+                                )
+                            )
+                        })?;
+                        Ok(fun.return_type(&new_types)?)
+                    }


Maybe we should bury this check, and conversion inside to the fun.return_type implementation for WindowFunctionDefinition::AggregateUDF not sure though.

I prefer separate coerce_types and return_types given the difference between these two

mustafasrepo · 2024-05-31T07:17:41Z

datafusion/expr/src/expr_schema.rs

+                        let new_types = data_types_with_aggregate_udf(&data_types, fun).map_err(|err| {
+                            plan_datafusion_err!(
+                                "{} and {}",
+                                err,
+                                utils::generate_signature_error_msg(
+                                    fun.name(),
+                                    fun.signature().clone(),
+                                    &data_types
+                                )
+                            )
+                        })?;


Similar comment above applies here

mustafasrepo · 2024-05-31T08:53:41Z

datafusion/physical-expr-common/src/aggregate/mod.rs

    }

    fn order_bys(&self) -> Option<&[PhysicalSortExpr]> {
-        (!self.ordering_req.is_empty()).then_some(&self.ordering_req)
+        if self.fun.has_ordering_requirements() && !self.ordering_req.is_empty() {


Even if has_ordering_requirements is not introduced in this PR. I think, with the order_sensitivity API. This API is redundant. I think, it is better to remove this API (in this Pr or in subsequent ones).

I agree. We can returns None for AggregateOrderSensitivity::Insensitive, ordering_req if others

Should we change the default order sensitivity to AggregateOrderSensitivity::Insensitive. For example, I think Sum would expect AggregateOrderSensitivity::Insensitive. And, probably only first/last, nth value and agg_order would expect other kinds of AggregateOrderSensitivity

I agree that, for most of the aggregate functions, AggregateOrderSensitivity::Insensitive is the correct behavior. However, I think the safest default choice is AggregateOrderSensitivity::HardRequirement. Also, as long as there is no requirement, default is not important. Hence, I think we can be strict in this choice.

Signed-off-by: jayzhan211 <[email protected]>

mustafasrepo

LGTM!. Thanks @jayzhan211 for this PR.

jayzhan211 · 2024-06-03T11:43:19Z

🚀
Thanks @mustafasrepo and @alamb

alamb · 2024-06-03T19:23:49Z

Are we tracking the follow on work (to remove the built in sum) anywhere?

appletreeisyellow · 2024-06-10T21:54:01Z

datafusion/physical-expr/src/aggregate/build_in.rs

+        (AggregateFunction::Sum, _) => {
+            return internal_err!("Builtin Sum will be removed");
+        }


Hi @jayzhan211! I saw @alamb already asked the same question here last week. I'd like to follow-up and check if you have any plans to remove this?

@appletreeisyellow You can checkout main branch it should be removed in #10831

@jayzhan211 Thank you so much! 💯

* move accumulate Signed-off-by: jayzhan211 <[email protected]> * move prim_op Signed-off-by: jayzhan211 <[email protected]> * move test to slt Signed-off-by: jayzhan211 <[email protected]> * remove sum distinct Signed-off-by: jayzhan211 <[email protected]> * move sum aggregate Signed-off-by: jayzhan211 <[email protected]> * fix args Signed-off-by: jayzhan211 <[email protected]> * add sum Signed-off-by: jayzhan211 <[email protected]> * merge fix Signed-off-by: jayzhan211 <[email protected]> * fix sum sig Signed-off-by: jayzhan211 <[email protected]> * todo: wait ahash merge Signed-off-by: jayzhan211 <[email protected]> * rebase Signed-off-by: jayzhan211 <[email protected]> * disable ordering req by default Signed-off-by: jayzhan211 <[email protected]> * check arg count Signed-off-by: jayzhan211 <[email protected]> * rm old workflow Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix failed test Signed-off-by: jayzhan211 <[email protected]> * doc and fmt Signed-off-by: jayzhan211 <[email protected]> * check udaf first Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix ci Signed-off-by: jayzhan211 <[email protected]> * fix ci Signed-off-by: jayzhan211 <[email protected]> * fix ci Signed-off-by: jayzhan211 <[email protected]> * fix err msg AGAIN Signed-off-by: jayzhan211 <[email protected]> * rm sum in builtin test which covered in sql Signed-off-by: jayzhan211 <[email protected]> * proto for window with udaf Signed-off-by: jayzhan211 <[email protected]> * fix slt Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix err msg Signed-off-by: jayzhan211 <[email protected]> * fix exprfn Signed-off-by: jayzhan211 <[email protected]> * fix ciy Signed-off-by: jayzhan211 <[email protected]> * fix ci Signed-off-by: jayzhan211 <[email protected]> * rename first/last to lowercase Signed-off-by: jayzhan211 <[email protected]> * skip sum Signed-off-by: jayzhan211 <[email protected]> * fix firstvalue Signed-off-by: jayzhan211 <[email protected]> * clippy Signed-off-by: jayzhan211 <[email protected]> * add doc Signed-off-by: jayzhan211 <[email protected]> * rm has_ordering_req Signed-off-by: jayzhan211 <[email protected]> * default hard req Signed-off-by: jayzhan211 <[email protected]> * insensitive for sum Signed-off-by: jayzhan211 <[email protected]> * cleanup duplicate code Signed-off-by: jayzhan211 <[email protected]> * Re-introduce check --------- Signed-off-by: jayzhan211 <[email protected]> Co-authored-by: Mustafa Akur <[email protected]>

apache/datafusion#10651 Don't know if the built-in sum used to work on intervals, but the UDAF does not.

jayzhan211 added 14 commits May 5, 2024 10:59

move accumulate

192211f

Signed-off-by: jayzhan211 <[email protected]>

move prim_op

403ee1d

Signed-off-by: jayzhan211 <[email protected]>

move test to slt

5bcab35

Signed-off-by: jayzhan211 <[email protected]>

remove sum distinct

426f2ab

Signed-off-by: jayzhan211 <[email protected]>

Merge branch 'move-sum-test' into sum-udaf

3712e18

move sum aggregate

e080d37

Signed-off-by: jayzhan211 <[email protected]>

Merge remote-tracking branch 'upstream/main' into sum-udaf

2b6784c

fix args

64db75c

Signed-off-by: jayzhan211 <[email protected]>

Merge remote-tracking branch 'upstream/main' into sum-udaf

c086a22

add sum

a9a5423

Signed-off-by: jayzhan211 <[email protected]>

Merge remote-tracking branch 'upstream/main' into sum-udaf

201c90c

merge fix

3e9e7e9

Signed-off-by: jayzhan211 <[email protected]>

fix sum sig

22540b2

Signed-off-by: jayzhan211 <[email protected]>

todo: wait ahash merge

d9229db

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels May 24, 2024

jayzhan211 mentioned this pull request May 24, 2024

[Epic] Unify AggregateFunction Interface (remove built in list of AggregateFunction s), improve the system #8708

Open

jayzhan211 added 5 commits May 25, 2024 08:00

Merge remote-tracking branch 'upstream/main' into sum-udaf

518894a

rebase

ee068db

Signed-off-by: jayzhan211 <[email protected]>

disable ordering req by default

6224333

Signed-off-by: jayzhan211 <[email protected]>

check arg count

47ae11f

Signed-off-by: jayzhan211 <[email protected]>

rm old workflow

25dcb64

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 25, 2024

jayzhan211 added 4 commits May 25, 2024 12:27

fmt

d16f1b1

Signed-off-by: jayzhan211 <[email protected]>

fix failed test

5381f2d

Signed-off-by: jayzhan211 <[email protected]>

doc and fmt

b403331

Signed-off-by: jayzhan211 <[email protected]>

check udaf first

78a70b3

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added the sql SQL Planner label May 25, 2024

alamb approved these changes May 30, 2024

View reviewed changes

jayzhan211 mentioned this pull request May 30, 2024

Convert builtin Sum aggregate function to UDAF #10731

Closed

add doc

2492ba7

Signed-off-by: jayzhan211 <[email protected]>

mustafasrepo reviewed May 31, 2024

View reviewed changes

jayzhan211 mentioned this pull request May 31, 2024

FIRST/LAST_VALUE behavior changes #10732

Closed

mustafasrepo reviewed May 31, 2024

View reviewed changes

rm has_ordering_req

73573be

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as draft May 31, 2024 14:03

jayzhan211 added 2 commits May 31, 2024 22:09

default hard req

f2b3732

Signed-off-by: jayzhan211 <[email protected]>

insensitive for sum

2c0c52c

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as ready for review May 31, 2024 15:09

jayzhan211 and others added 2 commits June 3, 2024 09:31

cleanup duplicate code

62346dd

Signed-off-by: jayzhan211 <[email protected]>

Re-introduce check

a41fcc5

mustafasrepo approved these changes Jun 3, 2024

View reviewed changes

jayzhan211 merged commit 888504a into apache:main Jun 3, 2024
25 checks passed

appletreeisyellow mentioned this pull request Jun 3, 2024

Regression in first_value and last_value coercion #10781

Closed

This was referenced Jun 3, 2024

WIP(iox-10891): patched df upgrade 202-05-28 influxdata/arrow-datafusion#23

Closed

WIP(iox-10891): patched df upgrade 202-05-31 influxdata/arrow-datafusion#24

Closed

WIP(iox-10892): patched df upgrade 202-06-03 influxdata/arrow-datafusion#25

Closed

appletreeisyellow reviewed Jun 10, 2024

View reviewed changes

This was referenced Jun 14, 2024

Unclear error message when calling a function with no parameters. #10915

Closed

chore: update some error messages for clarity #10916

Merged

jayzhan211 mentioned this pull request Jul 2, 2024

Make Sum an UDAF #10096

Closed

vgapeyev added a commit to sdf-labs/sql-functions that referenced this pull request Aug 26, 2024

SUM aggregation moved from builtins to a UDAF.

9284287

apache/datafusion#10651 Don't know if the built-in sum used to work on intervals, but the UDAF does not.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Sum UDAF #10651

Introduce Sum UDAF #10651

jayzhan211 commented May 24, 2024 •

edited by alamb

Loading

alamb left a comment

alamb May 30, 2024

alamb May 30, 2024

alamb May 30, 2024

alamb May 30, 2024

mustafasrepo May 31, 2024

jayzhan211 Jun 3, 2024

mustafasrepo May 31, 2024

mustafasrepo May 31, 2024

jayzhan211 Jun 3, 2024 •

edited

Loading

mustafasrepo May 31, 2024

mustafasrepo May 31, 2024

jayzhan211 May 31, 2024

jayzhan211 May 31, 2024

mustafasrepo May 31, 2024

mustafasrepo left a comment

jayzhan211 commented Jun 3, 2024

alamb commented Jun 3, 2024

appletreeisyellow Jun 10, 2024

jayzhan211 Jun 11, 2024

appletreeisyellow Jun 11, 2024

Introduce Sum UDAF #10651

Introduce Sum UDAF #10651

Conversation

jayzhan211 commented May 24, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustafasrepo left a comment

Choose a reason for hiding this comment

jayzhan211 commented Jun 3, 2024

alamb commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented May 24, 2024 •

edited by alamb

Loading

jayzhan211 Jun 3, 2024 •

edited

Loading