Move First Value UDAF and builtin first / last function to `aggregate-functions` #9960

jayzhan211 · 2024-04-05T12:45:55Z

Which issue does this PR close?

Part of #8708

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

jayzhan211 · 2024-04-05T14:02:49Z

I think the next step is to make last value udaf, so we can replace builtin first/last.

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

alamb

Thank you @jayzhan211 -- this is looking sweet and close

Can we add a re-export in of this crate in datafusion (like https://github.com/apache/arrow-datafusion/blob/c8584557cdfa7c138ab9039ceac31323f48a44d3/datafusion/core/src/lib.rs#L530-L533)?

Otherwise I think this is basically ready to go. 🚀

alamb · 2024-04-05T18:54:03Z

Cargo.toml

@@ -73,6 +74,7 @@ chrono = { version = "0.4.34", default-features = false }
 ctor = "0.2.0"
 dashmap = "5.4.0"
 datafusion = { path = "datafusion/core", version = "37.0.0", default-features = false }
+datafusion-aggregate-functions = { path = "datafusion/aggregate-functions", version = "37.0.0" }


What do you think about calling the new crate datafusion-functions-aggregates (to mirror the naming of datafusion-functions and datafusion-functions-array)?

alamb · 2024-04-05T18:55:42Z

datafusion/core/src/execution/context/mod.rs

-                panic!("Failed to register UDAF: {}", err);
-            }
-        }
+        datafusion_aggregate_functions::register_all(&mut new_self)


alamb · 2024-04-05T18:56:15Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

@@ -612,6 +612,7 @@ async fn roundtrip_expr_api() -> Result<()> {
            lit(1),
        ),
        array_replace_all(make_array(vec![lit(1), lit(2), lit(3)]), lit(2), lit(4)),
+        // TODO: Add first value after built-in functions are deprecated


I don't understand why we would wait until the built in functions are deprecated 🤔

You are right. Sorry for leaving this confusing comment. I should have left it in the review comment, not in the code 😆

I plan to support the expr API with arguments like distinct, and ordering after I deprecated all the built-in functions.

However, I added a simple example with lit(1).

Upd: It seems I need to support expr with arguments before deprecated builtin

datafusion/aggregate-functions/src/first_last.rs

alamb · 2024-04-05T19:00:55Z

datafusion/aggregate-functions/src/first_last.rs

-    acc_args: AccumulatorArgs,
-) -> Result<Box<dyn Accumulator>> {
-    let mut all_sort_orders = vec![];
+/// TO BE DEPRECATED: Builtin FIRST_VALUE physical aggregate expression


I don't understand this comment -- what does it mean to be deprecated? What is the alternative?

Is the idea it will be replaced with a UDF version ?

I think it may be replaced with udaf

datafusion/aggregate-functions/src/macros.rs

datafusion/expr/src/utils.rs

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

jayzhan211 · 2024-04-06T06:41:59Z

Besides addressing comments, I also move AggregateFunctionExpr and create_aggregate_expr to functions-aggregate from physical-expr-common because I think they are specific to aggregate.

alamb

Thank you @jayzhan211 -- I think this looks really cool and could be merged.

I think it would be better to leave AggregateFunctionExpr and create_aggregate_expr in the physical-expr-core crate for reasons I mentioned but if you disagree or want to put them back as a follow on PR I think that would be ok too.

alamb · 2024-04-06T08:57:35Z

datafusion/functions-aggregate/src/first_last.rs

+        let mut all_sort_orders = vec![];
+
+        // Construct PhysicalSortExpr objects from Expr objects:
+        let mut sort_exprs = vec![];


alamb · 2024-04-06T09:00:52Z

datafusion/functions-aggregate/src/lib.rs

+/// Any implementation of this trait also needs to implement the
+/// `PartialEq<dyn Any>` to allows comparing equality between the
+/// trait objects.
+pub trait AggregateExpr: Send + Sync + Debug + PartialEq<dyn Any> {


I also move AggregateFunctionExpr and create_aggregate_expr to functions-aggregate from physical-expr-common

While I agree that these two structures are aggregate specific, I still think they should be in datafusion-physical-expr-common because that would allow people to use DataFusion and create their own aggregate functions without having to take DataFusion's built in aggregate functions

I see real value in keeping the APIs required to use datafusion separate from the actual implementations

I agree that we should avoid the user pulling out unnecessary things for them. I thought they would need builtin functions, but they probably don't. I will keep them in common

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

jayzhan211 · 2024-04-07T02:25:07Z

Thanks for your review @alamb.

jayzhan211 added 21 commits April 3, 2024 09:31

backup

0eaf289

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

move PhysicalExpr

5338f61

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

cleanup

450ae4b

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

move physical sort

3624964

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

cleanup dependencies

835f147

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

add readme

c5d80c8

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

disable doc test

7851de7

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

move column

f5aafb3

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

fmt

7bfc074

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

move aggregatexp

675d2fe

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

move other two utils

5220087

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

license

113a000

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

switch to ignore

fea87e3

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

move reverse order

06d87bc

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

rename to common

26e5782

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

cleanup

26f852c

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

Merge branch 'physical-expr-core' into move-agg-crate-2

65bf4a1

backup

2bc58c1

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

Merge branch 'physical-expr-core' into move-agg-crate-2

ae9db96

move acc to first value

30d5576

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

move builtin expr too

672edc7

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

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate labels Apr 5, 2024

jayzhan211 added 6 commits April 5, 2024 21:18

use macro

109b790

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

fmt

87d589f

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

fix doc

398e4e2

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

add todo

04c7f5e

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

rm comments

01a1ddf

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

rm unused

4871414

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

jayzhan211 added 2 commits April 5, 2024 21:54

rm unused code

1ef212b

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

change to private

b6d53a5

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

fix lock

9aa15a2

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

jayzhan211 marked this pull request as ready for review April 5, 2024 14:49

jayzhan211 requested review from alamb and mustafasrepo April 5, 2024 15:00

alamb reviewed Apr 5, 2024

View reviewed changes

This was referenced Apr 5, 2024

DataFusion weekly project plan (Andrew Lamb) - April 1, 2024 #9899

Closed

Minor: Update library documentation with new crates #9966

Merged

jayzhan211 added 4 commits April 6, 2024 07:42

cleanup

e90464b

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

cleanup

ece925f

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

support roundtrip

89ccc89

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

remmove old format state

41a830a

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

jayzhan211 requested a review from alamb April 6, 2024 02:24

jayzhan211 marked this pull request as draft April 6, 2024 05:42

jayzhan211 marked this pull request as ready for review April 6, 2024 05:43

move aggregate related things to aggr crate

d235d2a

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

jayzhan211 mentioned this pull request Apr 6, 2024

Move conversion of FIRST/LAST Aggregate function to independent physical optimizer rule #9972

Closed

alamb approved these changes Apr 6, 2024

View reviewed changes

jayzhan211 added 5 commits April 6, 2024 20:06

move back to common

51cd272

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

taplo

38b2ce7

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

rm comment

9c7767c

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

cleanup

ea4adde

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

lock

39c5d15

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

jayzhan211 merged commit 2f1c3ab into apache:main Apr 7, 2024
25 checks passed

alamb mentioned this pull request Apr 8, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 #10002

Closed

9 tasks

waynexia mentioned this pull request May 31, 2024

FIRST/LAST_VALUE behavior changes #10732

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move First Value UDAF and builtin first / last function to `aggregate-functions` #9960

Move First Value UDAF and builtin first / last function to `aggregate-functions` #9960

jayzhan211 commented Apr 5, 2024

jayzhan211 commented Apr 5, 2024

alamb left a comment

alamb Apr 5, 2024

alamb Apr 5, 2024

alamb Apr 5, 2024

jayzhan211 Apr 6, 2024 •

edited

Loading

jayzhan211 Apr 6, 2024

alamb Apr 5, 2024

jayzhan211 Apr 5, 2024

jayzhan211 commented Apr 6, 2024 •

edited

Loading

alamb left a comment

alamb Apr 6, 2024

alamb Apr 6, 2024

jayzhan211 Apr 6, 2024 •

edited

Loading

jayzhan211 commented Apr 7, 2024

Move First Value UDAF and builtin first / last function to aggregate-functions #9960

Move First Value UDAF and builtin first / last function to aggregate-functions #9960

Conversation

jayzhan211 commented Apr 5, 2024

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?

jayzhan211 commented Apr 5, 2024

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

jayzhan211 Apr 6, 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

jayzhan211 commented Apr 6, 2024 • edited Loading

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

jayzhan211 Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Apr 7, 2024

Move First Value UDAF and builtin first / last function to `aggregate-functions` #9960

Move First Value UDAF and builtin first / last function to `aggregate-functions` #9960

jayzhan211 Apr 6, 2024 •

edited

Loading

jayzhan211 commented Apr 6, 2024 •

edited

Loading

jayzhan211 Apr 6, 2024 •

edited

Loading