Simplify windows builtin functions return type #8920

comphead · 2024-01-20T00:26:11Z

Which issue does this PR close?

Closes #.

Rationale for this change

Before the PR the Datafusion derives the output datatypes for builtin functions twice.

Initially set in BuiltInWindowFunction::return_type the output datatype redefined in other places:

in built in function implementations, like row_number, rank, etc
on physical planner phase like lag, lead

This PR is to unify output types to be defined in 1 single place and be transferred through the schema.

Another good benefit of it is other systems that embed DataFusion as an engine can now use its datatypes by constructing expected schema.

What changes are included in this PR?

Datatype definition unified, covered with new tests, some other minor optimizations

Are these changes tested?

Yes

Are there any user-facing changes?

No

alamb

This PR is to unify output types to be defined in 1 single place and be transferred through the schema.

Thank you @comphead -- this change looks good to me.

Before we merge it, however, I think we should give @mustafasrepo / @ozankabak a chance to comment as I think they are familiar with this code

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

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

alamb · 2024-01-21T12:39:41Z

datafusion/sqllogictest/test_files/window.slt

@@ -3906,3 +3906,69 @@ ProjectionExec: expr=[sn@0 as sn, ts@1 as ts, currency@2 as currency, amount@3 a
 --BoundedWindowAggExec: wdw=[SUM(table_with_pk.amount) ORDER BY [table_with_pk.sn ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Ok(Field { name: "SUM(table_with_pk.amount) ORDER BY [table_with_pk.sn ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: CurrentRow }], mode=[Sorted]
 ----SortExec: expr=[sn@0 ASC NULLS LAST]
 ------MemoryExec: partitions=1, partition_sizes=[1]
+


These tests all seem to pass for me on main as well (without the changes in this PR). Is that expected?

Yes, tests checked the functionality is not regressed.

alamb · 2024-01-21T12:42:24Z

datafusion/core/src/physical_planner.rs

@@ -719,14 +720,16 @@ impl DefaultPhysicalPlanner {
                    }

                    let logical_input_schema = input.schema();
-                    let physical_input_schema = input_exec.schema();
+                    // Extend the schema to include window expression fields as builtin window functions derives its datatype from incoming schema


Shouldn't input.schema() reflect all the columns that the input produces?

Or does the WindowAggExec create new columns "internally" by evaluating the window expressions?

input schema is schema from previous plan node(no window expressions). afaik windows expression column being added separately

https://github.com/apache/arrow-datafusion/blob/0116e2a9b4a3ed4491802e19195769b96b7a971a/datafusion/expr/src/logical_plan/plan.rs#L2045

ozankabak · 2024-01-21T13:55:01Z

@mustafasrepo PTAL

mustafasrepo · 2024-01-22T08:33:58Z

I think this PR add two functionality. Which are

Removing hard coded output types from builtin window functions so that all handled using return_type API.
Removing physical_input_schema from create_window_expr function.

I think first change is better. However, second change is misleading. With the changes in this PR, given logical_input_schema is no longer actual input schema, but really window schema.
However, I think we can merge this PR as is, Then I will file a PR to retract 2nd part. We can discuss pros and cons of it in that PR.

mustafasrepo · 2024-01-22T08:47:15Z

Filed the PR8945 for retracting second change

viirya · 2024-01-22T22:02:42Z

datafusion/core/src/physical_planner.rs

+                    let mut window_fields = logical_input_schema.fields().clone();
+                    window_fields.extend_from_slice(&exprlist_to_fields(window_expr.iter(), input)?);
+                    let extended_schema = &DFSchema::new_with_metadata(window_fields, HashMap::new())?;
                    let window_expr = window_expr
                        .iter()
                        .map(|e| {
                            create_window_expr(
                                e,
-                                logical_input_schema,
-                                &physical_input_schema,
+                                extended_schema,


Why do you need to recompute this extended_schema which looks like the same of window operator's schema? You can simply get the window's schema.

Filed #8955

viirya · 2024-01-22T22:04:28Z

datafusion/core/src/physical_planner.rs

@@ -1598,21 +1602,14 @@ pub fn create_window_expr_with_name(
 pub fn create_window_expr(
    e: &Expr,
    logical_input_schema: &DFSchema,


This is misleading, it is actually window's schema, not input schema now.

viirya · 2024-01-22T22:08:24Z

datafusion/core/src/physical_planner.rs

@@ -1572,7 +1575,8 @@ pub fn create_window_expr_with_name(
                    create_physical_sort_expr(e, logical_input_schema, execution_props)


Hmm, this looks incorrect as you use window's schema as input schema. Although window's schema is input schema + window functions output, it is why this change still makes thing work. But it is actually misleading for readers and probably cause of potential bugs.

Hmm, this looks incorrect as you use window's schema as input schema. Although window's schema is input schema + window functions output, it is why this change still makes thing work. But it is actually misleading for readers and probably cause of potential bugs.

This may be what @mustafasrepo has improved in #8920 (comment)

Hmm, this looks incorrect as you use window's schema as input schema. Although window's schema is input schema + window functions output, it is why this change still makes thing work. But it is actually misleading for readers and probably cause of potential bugs.

This may be what @mustafasrepo has improved in #8920 (comment)

Exactly, I re-introduced the invariant of using only input schema

Simplify windows builtin functions

54df958

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

comphead requested review from viirya and alamb January 20, 2024 00:27

alamb mentioned this pull request Jan 21, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

alamb approved these changes Jan 21, 2024

View reviewed changes

add field comments

91cd25e

mustafasrepo merged commit 2b218be into apache:main Jan 22, 2024
22 checks passed

mustafasrepo mentioned this pull request Jan 22, 2024

[MINOR]: Update create_window_expr to refer only input schema #8945

Merged

viirya reviewed Jan 22, 2024

View reviewed changes

comphead mentioned this pull request Jan 22, 2024

Simplify windows schema calculation #8955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify windows builtin functions return type #8920

Simplify windows builtin functions return type #8920

comphead commented Jan 20, 2024 •

edited

Loading

alamb left a comment

alamb Jan 21, 2024

comphead Jan 21, 2024

alamb Jan 21, 2024

comphead Jan 21, 2024

ozankabak commented Jan 21, 2024

mustafasrepo commented Jan 22, 2024 •

edited

Loading

mustafasrepo commented Jan 22, 2024

viirya Jan 22, 2024 •

edited

Loading

comphead Jan 22, 2024

viirya Jan 22, 2024

viirya Jan 22, 2024

alamb Jan 22, 2024

mustafasrepo Jan 23, 2024

		@@ -1572,7 +1575,8 @@ pub fn create_window_expr_with_name(
		create_physical_sort_expr(e, logical_input_schema, execution_props)

Simplify windows builtin functions return type #8920

Simplify windows builtin functions return type #8920

Conversation

comphead commented Jan 20, 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?

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

ozankabak commented Jan 21, 2024

mustafasrepo commented Jan 22, 2024 • edited Loading

mustafasrepo commented Jan 22, 2024

viirya Jan 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Jan 20, 2024 •

edited

Loading

mustafasrepo commented Jan 22, 2024 •

edited

Loading

viirya Jan 22, 2024 •

edited

Loading