Convert `BuiltInWindowFunction::{Lead, Lag}` to a user defined window function #12857

jcsherin · 2024-10-10T17:47:49Z

Which issue does this PR close?

Closes #12802.

Rationale for this change

Same as #12802.

What changes are included in this PR?

Converts lead and lag to user-defined window functions.
Adds support for serializing logical plan for window expressions with more than 1 argument.

Are these changes tested?

Adds logical plan roundtrip tests.

Are there any user-facing changes?

Yes.

Fixes: doc test build errors

jcsherin · 2024-10-15T15:08:00Z

Thanks @alamb 🙏

alamb · 2024-10-16T14:38:15Z

I merged this branch up to resolve some conflicts (due to #12893)

jcsherin · 2024-10-16T16:07:33Z

@alamb Before we merge this PR, I think it will be better to merge the bug fix done in BuiltInWindowFunction::{Lead,Lag} here:

Fix: handle NULL input in lead/lag window function #12811

I can port the bug fix over to this same PR after #12811 is merged. After that we can merge this to main.

Is that ok?

alamb · 2024-10-16T16:19:53Z

@alamb Before we merge this PR, I think it will be better to merge the bug fix done in BuiltInWindowFunction::{Lead,Lag} here:

Fix: handle NULL input in lead/lag window function #12811

I can port the bug fix over to this same PR after #12811 is merged. After that we can merge this to main.

Is that ok?

Sounds like a plan. I just merged #12811 -- just let me know when this PR is ready

alamb · 2024-10-16T16:20:11Z

Marking as draft so we don't accidentally merge it before the fixes are complete

alamb · 2024-10-16T18:31:36Z

CI tests are still failing it seems

jcsherin · 2024-10-16T18:39:16Z

datafusion/functions-window/src/lead_lag.rs

+        // See https://github.com/apache/datafusion/pull/12811
+        let (_expr, return_type) = rewrite_null_expr_and_data_type(
+            partition_evaluator_args.input_exprs(),
+            return_type,
+        )?;


Ported from Fix: handle NULL input in lead/lag window function #12811

The above bug fix in BuiltInWindowFunction:{Lead, Lag} rewrites the NULL expression to match the type of default value (if provided).

Right now this feature for overwriting arguments to user-defined window function is missing in WindowUDF. So for the moment this fix cannot be applied and the newly added regression sqllogictests will fail.

I'll have to look into bridging the gap between WindowUDF and BuiltInWindowFunction in this case.

jcsherin · 2024-10-16T19:01:32Z

@alamb

CI tests are still failing it seems

The bug fix relies on rewriting the NULL expression passed as argument to lead or lag to match the data type of the default value.

Right now this feature is missing in WindowUDF. To be specific, the expressions method in BuiltInWindowFunctionExpr:

datafusion/datafusion/physical-expr/src/window/nth_value.rs

Lines 115 to 117 in 16589b5

    
           fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> { 
        
               vec![Arc::clone(&self.expr)] 
        
           }

I think we need to add a similar API in WindowUDF to complete this bugfix.

For now I have disabled the new sqllogictests and marked them as TODO in 270a203.

I'll do a follow on PR to add this feature and squash the bug (again). Hope that is alright?

alamb · 2024-10-16T19:07:28Z

I'll do a follow on PR to add this feature and squash the bug (again). Hope that is alright?

Yes, thank you

alamb · 2024-10-16T19:09:15Z

Or maybe you could just make the change in this PR - the downside of merging this is that it reintroduces a bug (temporarily). I think given we are planning the 43 release #12470 in the next few days it would be nice to avoid breaking a previous bug fix

jcsherin · 2024-10-16T19:11:24Z

That works. Will keep this PR as draft until the bug fix is complete. Thanks 👍

This reverts commit 270a203.

jcsherin · 2024-10-17T16:36:52Z

Port the bug fix for handling NULL input in 814a9b7. Made it into a single commit to make it easier to see the isolated diff for the bug fix.
Added a few more test cases in c4b8840
Added WindowUDFImpl::expressions. It has a default implementation so does not break existing user-defined window function implementations.

@alamb This is ready for review again.

alamb

Makes sense to me -- thank you (again) @jcsherin

alamb · 2024-10-17T20:42:14Z

datafusion/functions-window-common/src/expr.rs

+use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
+use std::sync::Arc;
+
+/// Arguments passed to user-defined window function


jcsherin added 30 commits October 2, 2024 17:39

Move lead-lag to functions-window package

9efb417

Builds with warnings

6f05a9c

Adds PartitionEvaluatorArgs

d2ebd3a

Extracts shift_offset from input expressions

e5e5ab9

Computes shift offset

303b74b

Get default value from input expression

689ed3a

Implements partition_evaluator

0774a58

Fixes compiler warnings

009a1be

Comments out failing tests

8dc161f

Fixes cargo test errors and warnings

45e259e

Minor: taplo formatting

0d2aa99

Delete code

5ccb795

Define lead, lag user-defined window functions

7d8b3e6

Fixes cargo build errors

ba9d24a

Export udwf and expression public APIs

402dcac

Mark result field as nullable

2ce0883

Delete return_type tests for lead and lag

95e8f87

Disables test: window function case insensitive

04f30ec

Fixes: lowercase name in logical plan

d034082

Reverts to old methods for computing shift_offset, default_value

513df2a

Implements expression reversal

bb0bd8b

Fixes: lowercase name in logical plans

0e576bb

Fixes: doc test compilation errors

5490860

Fixes: doc test build errors

Temporarily quite clippy errors

f0c0e72

Fixes proto defintion

9051d2f

Merge branch 'main' into convert-lead-lag-udwf

b4cdc10

Minor: fixes formatting

be74322

Fixes: doc tests

c9ac517

Uses macro for defining lag_udwf() and leag_udwf()

e37752f

Fixes: window fuzz test cases

6fb12e6

alamb added the api change Changes the API exposed to users of the crate label Oct 15, 2024

Merge remote-tracking branch 'apache/main' into convert-lead-lag-udwf

7d00e3a

alamb marked this pull request as draft October 16, 2024 16:19

jcsherin added 2 commits October 16, 2024 22:27

Merge branch 'main' into convert-lead-lag-udwf

47c2b9c

Move null argument handling helper to utils

4385f21

alamb marked this pull request as ready for review October 16, 2024 18:31

alamb marked this pull request as draft October 16, 2024 18:31

jcsherin commented Oct 16, 2024

View reviewed changes

Disable failing sqllogic tests for handling NULL input

270a203

jcsherin added 3 commits October 17, 2024 13:11

Revert "Disable failing sqllogic tests for handling NULL input"

93569ed

This reverts commit 270a203.

Fixes: incorrect NULL handling in lead/lag window function

814a9b7

Adds more tests cases

c4b8840

alamb marked this pull request as ready for review October 17, 2024 20:41

alamb approved these changes Oct 18, 2024

View reviewed changes

alamb merged commit efe5708 into apache:main Oct 18, 2024
26 checks passed

jcsherin deleted the convert-lead-lag-udwf branch October 18, 2024 12:01

alamb mentioned this pull request Oct 21, 2024

Oct 21, 2024: This week in DataFusion #13035

Closed

4 tasks

This was referenced Oct 29, 2024

chore: re-export functions_window_common::ExpressionArgs #13149

Merged

default impl for WindowUDFImpl::expressions should use all input expressions #13168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `BuiltInWindowFunction::{Lead, Lag}` to a user defined window function #12857

Convert `BuiltInWindowFunction::{Lead, Lag}` to a user defined window function #12857

jcsherin commented Oct 10, 2024 •

edited

Loading

jcsherin commented Oct 15, 2024

alamb commented Oct 16, 2024

jcsherin commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

jcsherin Oct 16, 2024

jcsherin commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

jcsherin commented Oct 16, 2024

jcsherin commented Oct 17, 2024

alamb left a comment

alamb Oct 17, 2024

Convert BuiltInWindowFunction::{Lead, Lag} to a user defined window function #12857

Convert BuiltInWindowFunction::{Lead, Lag} to a user defined window function #12857

Conversation

jcsherin commented Oct 10, 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 commented Oct 15, 2024

alamb commented Oct 16, 2024

jcsherin commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

jcsherin Oct 16, 2024

Choose a reason for hiding this comment

jcsherin commented Oct 16, 2024

alamb commented Oct 16, 2024

alamb commented Oct 16, 2024

jcsherin commented Oct 16, 2024

jcsherin commented Oct 17, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 17, 2024

Choose a reason for hiding this comment

Convert `BuiltInWindowFunction::{Lead, Lag}` to a user defined window function #12857

Convert `BuiltInWindowFunction::{Lead, Lag}` to a user defined window function #12857

jcsherin commented Oct 10, 2024 •

edited

Loading