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 <jayzhan211@gmail.com>

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

    // 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

        &[]
    }

+    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

    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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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

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 <jayzhan211@gmail.com>

move prim_op

403ee1d

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

move test to slt

5bcab35

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

remove sum distinct

426f2ab

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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

3712e18

move sum aggregate

e080d37

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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

2b6784c

fix args

64db75c

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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

c086a22

add sum

a9a5423

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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

201c90c

merge fix

3e9e7e9

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

fix sum sig

22540b2

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

todo: wait ahash merge

d9229db

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates 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

Closed

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 <jayzhan211@gmail.com>

disable ordering req by default

6224333

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

check arg count

47ae11f

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

rm old workflow

25dcb64

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

fix failed test

5381f2d

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

doc and fmt

b403331

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

check udaf first

78a70b3

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

insensitive for sum

2c0c52c

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

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 <jayzhan211@gmail.com>

Re-introduce check

a41fcc5

mustafasrepo approved these changes Jun 3, 2024

View reviewed changes

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

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.

Introduce Sum UDAF #10651

Introduce Sum UDAF #10651

Uh oh!

Conversation

jayzhan211 commented May 24, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mustafasrepo left a comment

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Jun 3, 2024

Uh oh!

alamb commented Jun 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jayzhan211 commented May 24, 2024 •

edited by alamb

Loading

jayzhan211 Jun 3, 2024 •

edited

Loading