Convert `nth_value` to UDAF #11287

jcsherin · 2024-07-05T17:27:37Z

Which issue does this PR close?

Closes #11284.

Rationale for this change

What changes are included in this PR?

Converts nth_value to UDAF
Expression can be reversed so that N will be from the end instead of the start
When nth_value is used in window aggregations the existing builtin function is used instead of this UDAF. An explicit bypass is done where the logical plan nodes are created. This is similar to how it works for first_value and last_value.

Are these changes tested?

Test in roundtrip_physical_plan is updated
Use of lowercase naming in sqllogictest plans

Are there any user-facing changes?

Pending methods are: - `accumulator` - `state_fields` - `reverse_expr`

This error message is manually formatted to remain consistent with existing error statements. It is not formatted by running: ``` cargo test -p datafusion-sqllogictest --test sqllogictests errors -- --complete ```

This reverts commit 1913efda49e585816286b54b371d4166ac894d1f.

This reverts commit 4bb204f6ec8028c4e3313db5af3fabfcdaf7fea8.

This reverts commit fa61ce62dcae6eae6f8e9c9900ebf8cff5023bc0.

datafusion/functions-aggregate/src/nth_value.rs

jcsherin · 2024-07-05T17:33:56Z

datafusion/functions-aggregate/src/nth_value.rs

-            self.nullable, // This should be the same as field()
+            format_state_name(self.name(), "nth_value"),
+            Field::new("item", args.input_type.clone(), true),
+            false,


Should nullable be configurable? But it is unavailable in StateFieldArgs. I think it is related to #11274 and #11094.

I think given the existing nth function, we should let nullable configurable. And, the nullability is actually for the list element. We should add nullable in StateFieldArgs.

let mut fields = vec![Field::new_list( format_state_name(self.name(), "nth_value"), Field::new("item", args.input_type.clone(), self.nullable), false)]

@eejbyfeldt is working on it in #11063

Marked it as a TODO in comments so that it can be completed once it is added to StateFieldArgs.

based on #11299, I think we will have non-null for nth_value too (similar to first value).

datafusion/functions-aggregate/src/nth_value.rs

datafusion/sql/src/expr/function.rs

jayzhan211 · 2024-07-06T01:15:28Z

datafusion/functions-aggregate/src/nth_value.rs

-            self.nullable, // This should be the same as field()
+            format_state_name(self.name(), "nth_value"),
+            Field::new("item", args.input_type.clone(), true),
+            false,


I think given the existing nth function, we should let nullable configurable. And, the nullability is actually for the list element. We should add nullable in StateFieldArgs.

let mut fields = vec![Field::new_list( format_state_name(self.name(), "nth_value"), Field::new("item", args.input_type.clone(), self.nullable), false)]

@eejbyfeldt is working on it in #11063

datafusion/proto/tests/cases/roundtrip_physical_plan.rs

datafusion/sql/src/expr/function.rs

datafusion/functions-aggregate/src/nth_value.rs

jcsherin · 2024-07-06T10:21:15Z

Summary:

Marked as TODO: make the nullability of list field element configurable. This can be completed in a follow up PR after Add input_nullable for UDAF args StateField and Accumulator #11063.
Extract merge_ordered_arrays to physical-expr-common. It is required by both nth_value and array_agg_ordered. This can be moved back into functions-aggregate after the remaining functions are converted.
Simplify argument by using nth_value_udaf in roundtrip_physical_plan.

@jayzhan211 Thank you for the code review and providing context. It was very helpful.

jayzhan211

👍

alamb

Looks good to me too -- thank you @jcsherin and @jayzhan211

cc @mustafasrepo

alamb · 2024-07-07T19:49:22Z

datafusion/expr/src/aggregate_function.rs

@@ -39,8 +39,6 @@ pub enum AggregateFunction {
    Max,


this list is quite close to empty 🤞

alamb · 2024-07-07T19:49:43Z

datafusion/functions-aggregate/src/nth_value.rs

-    ordering_req: LexOrdering,
+    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.


alamb · 2024-07-08T16:43:40Z

I merged this PR up from main to ensure all the CI still passes and once they do I plan to merge it

alamb · 2024-07-08T17:37:42Z

🚀

alamb · 2024-07-08T17:37:49Z

Thanks again @jayzhan211 and @jcsherin

* Copies `NthValueAccumulator` to `functions-aggregate` * Partial implementation of `AggregateUDFImpl` Pending methods are: - `accumulator` - `state_fields` - `reverse_expr` * Implements `accumulator` method * Retains existing comments verbatim * Removes unnecessary path prefix * Implements `reverse_expr` method * Adds `nullable` field to `NthValue` * Revert to existing name * Implements `state_fields` method * Removes `nth_value` from `physical-expr` * Adds default * Exports `nth_value` * Fixes build error in physical plan roundtrip test * Minor: formatting * Parses `N` from input expression * Fixes build error by using `nth_value_udaf` * Fixes `reverse_expr` by passing correct `N` * Update plan with lowercase UDF name * Updates error message for incorrect no. of arguments This error message is manually formatted to remain consistent with existing error statements. It is not formatted by running: ``` cargo test -p datafusion-sqllogictest --test sqllogictests errors -- --complete ``` * Fixes nullable "item" in `state_fields` * Minor: fix formatting after resolving conflicts * Updates multiple existing plans with lowercase name * Implements `retract_batch` for window aggregations * Fixes: regex mismatch for error message in CI * Revert "Updates multiple existing plans with lowercase name" This reverts commit 1913efda49e585816286b54b371d4166ac894d1f. * Revert "Implements `retract_batch` for window aggregations" This reverts commit 4bb204f6ec8028c4e3313db5af3fabfcdaf7fea8. * Fixes: use builtin window function instead of udaf * Revert "Updates error message for incorrect no. of arguments" This reverts commit fa61ce62dcae6eae6f8e9c9900ebf8cff5023bc0. * Refactor: renames field and method * Removes hack for nullability * Minor: refactors `reverse_expr` * Minor: removes unncessary path prefix * Minor: cleanup arguments for creating aggregate expr * Refactor: extracts `merge_ordered_arrays` to `physical-expr-common` * Minor: adds todo for configuring nullability * Retrigger CI --------- Co-authored-by: Andrew Lamb <[email protected]>

jcsherin added 30 commits July 5, 2024 21:32

Copies NthValueAccumulator to functions-aggregate

bef3ede

Partial implementation of AggregateUDFImpl

c684040

Pending methods are: - `accumulator` - `state_fields` - `reverse_expr`

Implements accumulator method

165c4f5

Retains existing comments verbatim

e82f055

Removes unnecessary path prefix

a45349f

Implements reverse_expr method

9c9a6c4

Adds nullable field to NthValue

60370ca

Revert to existing name

3631812

Implements state_fields method

98039e5

Removes nth_value from physical-expr

415d9db

Adds default

d9ebdbe

Exports nth_value

c4e5417

Fixes build error in physical plan roundtrip test

f74459b

Minor: formatting

0fc98f1

Parses N from input expression

02c01fc

Fixes build error by using nth_value_udaf

729d9c5

Fixes reverse_expr by passing correct N

40b4607

Update plan with lowercase UDF name

a86ca1f

Updates error message for incorrect no. of arguments

02f4497

This error message is manually formatted to remain consistent with existing error statements. It is not formatted by running: ``` cargo test -p datafusion-sqllogictest --test sqllogictests errors -- --complete ```

Fixes nullable "item" in state_fields

2e90028

Minor: fix formatting after resolving conflicts

972f118

Updates multiple existing plans with lowercase name

cf974c0

Implements retract_batch for window aggregations

427a8bb

Fixes: regex mismatch for error message in CI

488881f

Revert "Updates multiple existing plans with lowercase name"

91bacb7

This reverts commit 1913efda49e585816286b54b371d4166ac894d1f.

Revert "Implements retract_batch for window aggregations"

97e8955

This reverts commit 4bb204f6ec8028c4e3313db5af3fabfcdaf7fea8.

Fixes: use builtin window function instead of udaf

4634641

Revert "Updates error message for incorrect no. of arguments"

7b57cce

This reverts commit fa61ce62dcae6eae6f8e9c9900ebf8cff5023bc0.

Refactor: renames field and method

20a804b

Removes hack for nullability

cc08872

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jul 5, 2024

jcsherin commented Jul 5, 2024

View reviewed changes

datafusion/functions-aggregate/src/nth_value.rs Show resolved Hide resolved

jcsherin commented Jul 5, 2024

View reviewed changes

datafusion/functions-aggregate/src/nth_value.rs Outdated Show resolved Hide resolved

jcsherin commented Jul 5, 2024

View reviewed changes

datafusion/sql/src/expr/function.rs Show resolved Hide resolved

jcsherin changed the title ~~Convert udaf nth value~~ Convert nth_value to UDAF Jul 5, 2024

Minor: removes unncessary path prefix

f6215d9

jayzhan211 reviewed Jul 6, 2024

View reviewed changes

jcsherin added 3 commits July 6, 2024 14:15

Minor: cleanup arguments for creating aggregate expr

5874f0f

Refactor: extracts merge_ordered_arrays to physical-expr-common

6ce6679

Minor: adds todo for configuring nullability

5ea5421

jcsherin added 2 commits July 6, 2024 17:22

Retrigger CI

d0a3c3d

Merge branch 'main' into convert-udaf-nth-value

e3a8026

jayzhan211 approved these changes Jul 7, 2024

View reviewed changes

alamb approved these changes Jul 7, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into convert-udaf-nth-value

1990bad

alamb merged commit 4ac1428 into apache:main Jul 8, 2024
25 checks passed

alamb added the api change Changes the API exposed to users of the crate label Jul 9, 2024

jcsherin deleted the convert-udaf-nth-value branch July 11, 2024 18:32

jcsherin mentioned this pull request Jul 12, 2024

Add nullable in StateFieldArgs #11433

Closed

alamb mentioned this pull request Sep 20, 2024

[Epic] Unify WindowFunction Interface (remove built in list of BuiltInWindowFunction s) #8709

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `nth_value` to UDAF #11287

Convert `nth_value` to UDAF #11287

jcsherin commented Jul 5, 2024 •

edited

Loading

jcsherin Jul 5, 2024 •

edited

Loading

jayzhan211 Jul 6, 2024

jcsherin Jul 6, 2024 •

edited

Loading

jayzhan211 Jul 7, 2024

jayzhan211 Jul 6, 2024

jcsherin commented Jul 6, 2024

jayzhan211 left a comment

alamb left a comment

alamb Jul 7, 2024

alamb Jul 7, 2024

alamb commented Jul 8, 2024 •

edited

Loading

alamb commented Jul 8, 2024

alamb commented Jul 8, 2024

Convert nth_value to UDAF #11287

Convert nth_value to UDAF #11287

Conversation

jcsherin commented Jul 5, 2024 • edited 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?

jcsherin Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

jayzhan211 Jul 6, 2024

Choose a reason for hiding this comment

jcsherin Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

jayzhan211 Jul 7, 2024

Choose a reason for hiding this comment

jayzhan211 Jul 6, 2024

Choose a reason for hiding this comment

jcsherin commented Jul 6, 2024

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 7, 2024

Choose a reason for hiding this comment

alamb Jul 7, 2024

Choose a reason for hiding this comment

alamb commented Jul 8, 2024 • edited Loading

alamb commented Jul 8, 2024

alamb commented Jul 8, 2024

Convert `nth_value` to UDAF #11287

Convert `nth_value` to UDAF #11287

jcsherin commented Jul 5, 2024 •

edited

Loading

jcsherin Jul 5, 2024 •

edited

Loading

jcsherin Jul 6, 2024 •

edited

Loading

alamb commented Jul 8, 2024 •

edited

Loading