Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate #11564

jayzhan211 · 2024-07-20T09:05:03Z

Which issue does this PR close?

Part of #8708

Rationale for this change

Migrating DataFusion so there is no distinction between built in and user defined functions

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-07-20T09:10:49Z

datafusion/physical-expr-common/src/utils.rs

+            limited_convert_logical_expr_to_physical_expr_with_dfschema(expr, dfschema)?,
+        ),
+        Expr::Column(col) => {
+            let idx = dfschema.index_of_column(col)?;


I found that I need dfschema in order to get the correct field, so I change all the UDAF to create_aggregate_expr_with_dfschema which is used internally for UDAF only.

After removing limited_convert_logical_expr_to_physical_expr_with_dfschema, I think we don't need DFSchema anymore

So does that mean limited_convert_logical_expr_to_physical_expr_with_dfschema will be temporary?

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

jayzhan211 · 2024-07-22T01:01:39Z

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

+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(


first/last and ordered_array_agg are switched to this function. After #11359 , I think we can have a single create_aggregate_expr again.

Make we can also make a builder to make using this function easier (e.g. it might be easy to swap one of the last three bools by accident)

Great! I already filed the ticket in #11543.

alamb

Thank you @jayzhan211 -- we are getting very close 🤞

I do think some of the APIs are getting a little clumsy but perhaps we can refine them over time as follow on PRs (e.g. with builders, etc)

alamb · 2024-07-22T18:56:44Z

datafusion/expr/src/function.rs

@@ -57,6 +57,9 @@ pub struct AccumulatorArgs<'a> {
    /// The schema of the input arguments
    pub schema: &'a Schema,

+    /// The schema of the input arguments


I view this addition as basically making the UDAF framework as full featured as the built in one is

I hope we will have either dfschema or schema at the end 🤔

alamb · 2024-07-22T19:00:10Z

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

+///
+/// `is_reversed` is used to indicate whether the aggregation is running in reverse order,
+/// it could be used to hint Accumulator to accumulate in the reversed order,
+/// you can just set to false if you are not reversing expression
 #[allow(clippy::too_many_arguments)]
 pub fn create_aggregate_expr(


Given the number of arguments it is getting, maybe it would be good to define a builder like the following:

let builder = AggregateExprBuilder::new(fun), .with_input_phyiscal_exprs(input_phy_exprs) ... .with_name(name) .build()?

🤔

alamb · 2024-07-22T19:01:19Z

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

+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(


Make we can also make a builder to make using this function easier (e.g. it might be easy to swap one of the last three bools by accident)

alamb · 2024-07-22T19:01:38Z

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

@@ -495,18 +573,23 @@ impl AggregateExpr for AggregateFunctionExpr {
                    })
                    .collect::<Vec<_>>();
                let mut name = self.name().to_string();
-                replace_order_by_clause(&mut name);
+                // TODO: Generalize order-by clause rewrite


Should we track this in a follow on ticket?

alamb · 2024-07-22T19:02:16Z

datafusion/physical-expr-common/src/utils.rs

+            limited_convert_logical_expr_to_physical_expr_with_dfschema(expr, dfschema)?,
+        ),
+        Expr::Column(col) => {
+            let idx = dfschema.index_of_column(col)?;


So does that mean limited_convert_logical_expr_to_physical_expr_with_dfschema will be temporary?

jayzhan211 · 2024-07-22T23:56:50Z

limited_convert_logical_expr_to_physical_expr_with_dfschema and limited_convert_logical_expr_to_physical_expr are both temporary until we refactor #11359

jayzhan211 · 2024-07-23T00:01:48Z

TODO:

Remove ArrayAgg Builtin definition and lowercase the name
Generalize order-by clause rewrite
Expr Builder (not sure before the refactor or after 🤔 )

jayzhan211 · 2024-07-23T00:02:08Z

Thanks @alamb !

alamb · 2024-07-23T00:53:45Z

Thanks @alamb !

Thank you -- it is so close I can feel it

jayzhan211 added 3 commits July 20, 2024 15:46

first draft

a64f55b

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

rm old agg

e35adde

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

replace udaf with interal function - create aggregate with dfschema

a9b7d51

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

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 20, 2024

rm test

174f07c

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

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Jul 20, 2024

cleanup

889c0b8

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

jayzhan211 commented Jul 20, 2024

View reviewed changes

jayzhan211 added 3 commits July 20, 2024 17:17

rm useless

6d840b2

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

fix test

c8e1ca2

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

rename

3437f7a

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

jayzhan211 commented Jul 22, 2024

View reviewed changes

jayzhan211 marked this pull request as ready for review July 22, 2024 10:03

alamb approved these changes Jul 22, 2024

View reviewed changes

alamb changed the title ~~Move OrderSensitiveArrayAgg to UDAF~~ Migrate OrderSensitiveArrayAgg to be a user defined aggregate Jul 22, 2024

jayzhan211 merged commit d941dc3 into apache:main Jul 23, 2024
26 checks passed

jayzhan211 deleted the array-udf-order branch July 23, 2024 00:02

This was referenced Jul 26, 2024

Use AccumulatorArgs::is_reversed in NthValueAgg #11668

Closed

Use AccumulatorArgs::is_reversed in NthValueAgg #11669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate #11564

Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate #11564

jayzhan211 commented Jul 20, 2024 •

edited by alamb

Loading

jayzhan211 Jul 20, 2024

jayzhan211 Jul 22, 2024

alamb Jul 22, 2024

jayzhan211 Jul 22, 2024

alamb Jul 22, 2024

jayzhan211 Jul 22, 2024 •

edited

Loading

alamb left a comment

alamb Jul 22, 2024

jayzhan211 Jul 22, 2024

alamb Jul 22, 2024

alamb Jul 22, 2024

alamb Jul 22, 2024

alamb Jul 22, 2024

jayzhan211 commented Jul 22, 2024 •

edited

Loading

jayzhan211 commented Jul 23, 2024 •

edited

Loading

jayzhan211 commented Jul 23, 2024

alamb commented Jul 23, 2024 •

edited

Loading

Migrate OrderSensitiveArrayAgg to be a user defined aggregate #11564

Migrate OrderSensitiveArrayAgg to be a user defined aggregate #11564

Conversation

jayzhan211 commented Jul 20, 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?

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 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

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

jayzhan211 commented Jul 22, 2024 • edited Loading

jayzhan211 commented Jul 23, 2024 • edited Loading

jayzhan211 commented Jul 23, 2024

alamb commented Jul 23, 2024 • edited Loading

Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate #11564

Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate #11564

jayzhan211 commented Jul 20, 2024 •

edited by alamb

Loading

jayzhan211 Jul 22, 2024 •

edited

Loading

jayzhan211 commented Jul 22, 2024 •

edited

Loading

jayzhan211 commented Jul 23, 2024 •

edited

Loading

alamb commented Jul 23, 2024 •

edited

Loading