Add `field` trait method to `WindowUDFImpl`, remove `return_type`/`nullable` by jcsherin · Pull Request #12374 · apache/datafusion

jcsherin · 2024-09-07T13:35:28Z

Which issue does this PR close?

Rationale for this change

The result field from evaluating the user-defined window function is composed from the return_type and nullable trait methods in WindowUDFImpl.

This change explores folding both methods into a single trait method. The user-defined window functions have to implement only the field trait method which makes the intent more explicit.

The current implementation for a user-defined window function (without field trait method) looks like this:

impl WindowUDFImpl for RowNumber {
    fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
        Ok(DataType::UInt64)
    }

    fn nullable(&self) -> bool {
        true
    }
}

The implementation for a user-defined window function after this change:

impl WindowUDFImpl for RowNumber {
    fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field> {
        Ok(Field::new(
             field_args.name(), /* window function display name */
             DataType::UInt64,  /* result data type */
             false              /* row number is not nullable */
       ))
    }
}

What changes are included in this PR?

Add field trait method:

fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field>

Remove return_type trait method.

datafusion/datafusion/expr/src/udwf.rs

Lines 282 to 284 in a08f923

    
               /// What [`DataType`] will be returned by this function, given the types of 
        
               /// the arguments 
        
               fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;

Remove nullable trait method which was added in Convert built-in row_number to user-defined window function #12030.
Add WindowUDFFieldArgs:

/// Contains metadata necessary for defining the field which represents
/// the final result of evaluating a user-defined window function.
pub struct WindowUDFFieldArgs<'a> {
    /// The data types of input expressions to the user-defined window
    /// function.
    input_types: &'a [DataType],
    /// The display name of the user-defined window function.
    function_name: &'a str,
}

Are these changes tested?

Yes, against existing tests in CI.

Are there any user-facing changes?

Yes, this is a breaking change for user-defined window functions API.

jcsherin · 2024-09-17T13:21:41Z

datafusion/expr/src/expr_schema.rs

+impl Expr {
+    /// Common method for window functions that applies type coercion
+    /// to all arguments of the window function to check if it matches
+    /// its signature.
+    ///
+    /// If successful, this method returns the data type and
+    /// nullability of the window function's result.
+    ///
+    /// Otherwise, returns an error if there's a type mismatch between
+    /// the window function's signature and the provided arguments.
+    fn data_type_and_nullable_with_window_function(
+        &self,
+        schema: &dyn ExprSchema,
+        window_function: &WindowFunction,
+    ) -> Result<(DataType, bool)> {


Extracted a common method to handle type coercion for all window function types (built-in, udaf and udwf) which is then reused by methods:,

data_type_and_nullable,

get_type and,

nullable

jcsherin · 2024-09-17T13:22:36Z

datafusion/expr/src/udwf.rs

-    /// Return the type of the function given its input types
-    ///
-    /// See [`WindowUDFImpl::return_type`] for more details.
-    pub fn return_type(&self, args: &[DataType]) -> Result<DataType> {
-        self.inner.return_type(args)
-    }
-


Removed return_type.

jcsherin · 2024-09-17T13:22:48Z

datafusion/expr/src/udwf.rs

-    /// Returns if column values are nullable for this window function.
+    /// Returns the field of the final result of evaluating this window function.
    ///
-    /// See [`WindowUDFImpl::nullable`] for more details.
-    pub fn nullable(&self) -> bool {
-        self.inner.nullable()


Removed nullable

jcsherin · 2024-09-17T13:24:40Z

datafusion/expr/src/expr_schema.rs

+                            )
+                        )
+                    })?;
+                let (_, function_name) = self.qualified_name();


Use Expr::qualified_name which also handles:

Expr::Column and,

Expr::Alias

jcsherin · 2024-09-17T13:31:52Z

datafusion/expr/src/expr.rs

+            WindowFunctionDefinition::WindowUDF(fun) => fun
+                .field(WindowUDFFieldArgs::new(input_expr_types, display_name))
+                .map(|field| field.data_type().clone()),


Return data type for udwf.

jayzhan211

👍

jcsherin · 2024-09-19T09:56:23Z

Thanks @jayzhan211

Michael-J-Ward

EDIT:

If this is "too late" for this kind of comment, please let me know and I'll delete. I hadn't seen the issue / PR work until today.

One performance consideration is that Field::new allocates a new string on each invocation.

Could that be why WindowUDF (and both ScalarUDF and AggregateUDF) preferred to keep the methods separate?

I dug into it because needing to add empty &str to all the tests in datafusion/expr/src/expr.rs made me think it's not the right abstraction.

Lastly, do we want to diverge Aggregate and Window UDFs?

I thought the recent trend had been to unify them, like in this PR #11550 by @timsaucer?

jayzhan211 · 2024-09-20T07:44:07Z

One performance consideration is that Field::new allocates a new string on each invocation.

I don't think there is any issue given that we usually require the whole Field

Note that data_type_and_nullable is introduced because of performance, avoid to call the function twice to get data_type and nullalbe. I think it makes sense to have a single call to get the Field which includes data_type, nullable and name. If not, maybe it indicates we need to redesign the code to ensure it is called once.

Could that be why WindowUDF (and both ScalarUDF and AggregateUDF) preferred to keep the methods separate?

Maybe historically issue?

Lastly, do we want to diverge Aggregate and Window UDFs?

I don't think so, they should be separated, no any good reason to mix them

jcsherin · 2024-09-20T17:27:23Z

If this is "too late" for this kind of comment, please let me know and I'll delete. I hadn't seen the issue / PR work until today.

@Michael-J-Ward Appreciate the extra set of eyes. Thank you, for reviewing the code 🙌

I dug into it because needing to add empty &str to all the tests in datafusion/expr/src/expr.rs made me think it's not the right abstraction.

datafusion/datafusion/expr/src/expr.rs

Lines 704 to 724 in 5cc7d06

    
           impl WindowFunctionDefinition { 
        
               /// Returns the datatype of the window function 
        
               pub fn return_type( 
        
                   &self, 
        
                   input_expr_types: &[DataType], 
        
                   _input_expr_nullable: &[bool], 
        
                   display_name: &str, 
        
               ) -> Result<DataType> { 
        
                   match self { 
        
                       WindowFunctionDefinition::BuiltInWindowFunction(fun) => { 
        
                           fun.return_type(input_expr_types) 
        
                       } 
        
                       WindowFunctionDefinition::AggregateUDF(fun) => { 
        
                           fun.return_type(input_expr_types) 
        
                       } 
        
                       WindowFunctionDefinition::WindowUDF(fun) => fun 
        
                           .field(WindowUDFFieldArgs::new(input_expr_types, display_name)) 
        
                           .map(|field| field.data_type().clone()), 
        
                   } 
        
               }

The WindowFunctionDefintion::return_type is used only for tests. And I believe these unit tests do not provide much value.

When row_number() was converted to WindowUDF the corresponding return type unit test for row_number() was removed. And I think this applies to the remaining builtin window methods which are pending conversion to WindowUDF.

@Michael-J-Ward @jayzhan211 Thank you.

alamb

👨‍🍳 👌

Looks really nice to me -- thank you @jcsherin and @jayzhan211

I also have it on my list to file some more sub task tickets for #8709 to remove the rest of the built in WindowFunctions

alamb · 2024-09-21T11:23:14Z

Thanks again @jayzhan211 and @jcsherin

Blizzara · 2024-09-23T09:02:53Z

datafusion/expr/src/udwf.rs

-    fn nullable(&self) -> bool {
-        true
-    }
+    /// The [`Field`] of the final result of evaluating this window function.


Could be useful to document here how the "name" for the returned field is supposed to be set :)

Agreed. It's a great suggestion. I'll implement in a follow-up PR.

Thanks @Blizzara

…llable` (apache#12374) * Adds new library `functions-window-common` * Adds `FieldArgs` struct for field of final result * Adds `field` method to `WindowUDFImpl` trait * Minor: fixes formatting * Fixes: udwf doc test * Fixes: implements missing trait items * Updates `datafusion-cli` dependencies * Fixes: formatting of `Cargo.toml` files * Fixes: implementation of `field` in udwf example * Pass `FieldArgs` argument to `field` * Use `field` in place of `return_type` for udwf * Update `field` in udwf implementations * Fixes: implementation of `field` in udwf example * Revert unrelated change * Mark `return_type` for udwf as unreachable * Delete code * Uses schema name of udwf to construct `FieldArgs` * Adds deprecated notice to `return_type` trait method * Add doc comments to `field` trait method * Reify `input_types` when creating the udwf window expression * Rename name field to `schema_name` in `FieldArgs` * Make `FieldArgs` opaque * Minor refactor * Removes `nullable` trait method from `WindowUDFImpl` * Add doc comments * Rename to `WindowUDFResultArgs` * Minor: fixes formatting * Copy edits for doc comments * Renames field to `function_name` * Rename struct to `WindowUDFFieldArgs` * Add comments for unreachable code * Copy edit for `WindowUDFImpl::field` trait method * Renames module * Fix warning: unused doc comment * Minor: rename bindings * Minor refactor * Minor: copy edit * Fixes: use `Expr::qualified_name` for window function name * Fixes: apply previous fix to `Expr::nullable` * Refactor: reuse type coercion for window functions * Fixes: clippy errors * Adds name parameter to `WindowFunctionDefinition::return_type` * Removes `return_type` field from `SimpleWindowUDF` * Add doc comment for helper method * Rewrite doc comments * Minor: remove empty comment * Remove `WindowUDFImpl::return_type` * Fixes doc test

alamb · 2024-09-27T11:18:26Z

I also have it on my list to file some more sub task tickets for #8709 to remove the rest of the built in WindowFunctions

I filed a few more tickets to hopefully get this process started.

Convert BuiltInWindowFunction::{Rank, PercentRank, DenseRank} to a user defined functions #12648
Convert BuiltInWindowFunction::{NthValue} to a user defined functions #12649

jcsherin added 30 commits September 3, 2024 16:43

Adds new library functions-window-common

ae69e1e

Adds FieldArgs struct for field of final result

f09383c

Adds field method to WindowUDFImpl trait

ca256fa

Minor: fixes formatting

9b21b7f

Fixes: udwf doc test

9324df6

Fixes: implements missing trait items

4cd1467

Updates datafusion-cli dependencies

deff13c

Fixes: formatting of Cargo.toml files

ac5ef0a

Fixes: implementation of field in udwf example

7c4293a

Pass FieldArgs argument to field

83d833b

Use field in place of return_type for udwf

927f1cc

Update field in udwf implementations

e29a493

Merge branch 'main' into field-args-for-window-udf

79d8372

Fixes: implementation of field in udwf example

fc969e8

Revert unrelated change

df2b1c8

Mark return_type for udwf as unreachable

e26aba6

Delete code

258986d

Uses schema name of udwf to construct FieldArgs

ba9e39a

Adds deprecated notice to return_type trait method

b671b36

Add doc comments to field trait method

712263e

Reify input_types when creating the udwf window expression

9426978

Rename name field to schema_name in FieldArgs

327db32

Make FieldArgs opaque

f5b62ef

Minor refactor

34565ff

Removes nullable trait method from WindowUDFImpl

ede7827

Add doc comments

25f3b50

Rename to WindowUDFResultArgs

f012bdd

Minor: fixes formatting

0e8795f

Copy edits for doc comments

4bd77d5

Renames field to function_name

a0f2ccb

Rewrite doc comments

2530169

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

Minor: remove empty comment

25b299e

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

jcsherin added 2 commits September 17, 2024 18:11

Remove WindowUDFImpl::return_type

ce7df3a

Fixes doc test

bb57f8f

jcsherin commented Sep 17, 2024

View reviewed changes

jcsherin marked this pull request as ready for review September 17, 2024 14:27

jcsherin added 2 commits September 18, 2024 11:37

Merge branch 'main' into field-args-for-window-udf

e6a7113

Merge branch 'main' into field-args-for-window-udf

c627403

jayzhan211 approved these changes Sep 19, 2024

View reviewed changes

Michael-J-Ward reviewed Sep 19, 2024

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Sep 20, 2024

alamb changed the title ~~Add field trait method to WindowUDFImpl~~ Add field trait method to WindowUDFImpl, remove return_type Sep 20, 2024

alamb changed the title ~~Add field trait method to WindowUDFImpl, remove return_type~~ Add field trait method to WindowUDFImpl, remove return_type/nullable Sep 20, 2024

alamb approved these changes Sep 20, 2024

View reviewed changes

alamb merged commit e1b992a into apache:main Sep 21, 2024

Blizzara reviewed Sep 23, 2024

View reviewed changes

jcsherin deleted the field-args-for-window-udf branch September 23, 2024 18:51

jcsherin mentioned this pull request Oct 4, 2024

Doc how "name" for returned field is to be set #12756

Closed

	/// What [`DataType`] will be returned by this function, given the types of
	/// the arguments
	fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;

Conversation

jcsherin commented Sep 7, 2024 • edited 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!

jcsherin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jcsherin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jcsherin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jcsherin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jcsherin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

jcsherin commented Sep 19, 2024

Uh oh!

Michael-J-Ward left a comment • 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!

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

Uh oh!

Uh oh!

jcsherin commented Sep 20, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 21, 2024

Uh oh!

Blizzara Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

jcsherin Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcsherin commented Sep 7, 2024 •

edited

Loading

Michael-J-Ward left a comment •

edited

Loading

jayzhan211 commented Sep 20, 2024 •

edited

Loading