Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove BuiltInWindowFunction (LogicalPlans) #13393

Merged
merged 4 commits into from
Nov 15, 2024
Merged

Remove BuiltInWindowFunction (LogicalPlans) #13393

merged 4 commits into from
Nov 15, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Nov 13, 2024 •
edited
Loading

Which issue does this PR close?

Rationale for this change

@buraksen and @jcsherin ported the last window function over, so now we can remove the old BuiltInWindowFunction code

This was so tempting a cleanup that I had to give it a try (and I wanted to do some coding)

What changes are included in this PR?

Changes:

  1. Remove BuiltInWindowFunction
  2. Remove BuiltInWindowFunctionDefinition::BuiltInWindowFunction
  3. Remove Protobuf definitions
  4. Update code to compile

Are these changes tested?

Yes, by existing CI

@github-actions github-actions bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait common Related to common crate execution Related to the execution crate proto Related to proto crate functions labels Nov 13, 2024
@alamb alamb marked this pull request as draft November 13, 2024 15:57
@alamb
Copy link
Contributor Author

alamb commented Nov 13, 2024

There are actually several related "BuiltInWindowExpr" in physical-expr that need to be cleaned up as well (that are no longer related to BuiltInWindowExprs). We may also be able to remove a level of trait.

@alamb alamb force-pushed the alamb/remove_built_in_window branch from e6bb4c5 to a300ee5 Compare November 13, 2024 16:58
@alamb alamb changed the title Remove BuiltInWindowFunction Remove BuiltInWindowFunction (LogicalPlans) Nov 13, 2024
@jonathanc-n
Copy link
Contributor

@alamb @buraksenn @Omega359 Pretty sure we can now clean up the old window docs, due to the nth_value migration being merged. Lemme see if I can do that really quick

@alamb
Copy link
Contributor Author

alamb commented Nov 13, 2024

@alamb @buraksenn @Omega359 Pretty sure we can now clean up the old window docs, due to the nth_value migration being merged. Lemme see if I can do that really quick

Yes, we can -- I think that is tracked by

@github-actions github-actions bot removed documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait common Related to common crate execution Related to the execution crate labels Nov 13, 2024
@alamb alamb marked this pull request as ready for review November 14, 2024 07:06
jcsherin
jcsherin reviewed Nov 14, 2024
View reviewed changes
datafusion/expr/src/expr.rs Outdated
///
/// In SQL, you can use:
/// - Actual window functions ([`WindowUDF`])
/// - Noraml aggregate functions ([`AggregateUDF`])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here: "Normal"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6b1a243

@jcsherin
Copy link
Contributor

LGTM 👍. Thanks @alamb

crepererum
crepererum approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@crepererum crepererum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

alamb
alamb commented Nov 14, 2024
View reviewed changes
datafusion/expr/src/udwf.rs
@@ -39,8 +39,16 @@ use datafusion_functions_window_common::field::WindowUDFFieldArgs;
use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
use datafusion_physical_expr_common::physical_expr::PhysicalExpr;

/// Logical representation of a user-defined window function (UDWF)
/// A UDWF is different from a UDF in that it is stateful across batches.
/// Logical representation of a user-defined window function (UDWF).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also threw in some drive by documentation improvements to sweeten the deal 🍯

datafusion/physical-plan/src/windows/mod.rs
@@ -120,7 +117,6 @@ pub fn create_window_expr(
aggregate,
)
}
// TODO: Ordering not supported for Window UDFs yet
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ORDER BY is already supported as the order_by clause is passed to window_expr_from_aggregate_expr

datafusion/expr/src/expr.rs Outdated
///
/// In SQL, you can use:
/// - Actual window functions ([`WindowUDF`])
/// - Noraml aggregate functions ([`AggregateUDF`])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6b1a243

@alamb alamb merged commit 75a27a8 into apache:main Nov 15, 2024
25 checks passed
@alamb
Copy link
Contributor Author

alamb commented Nov 15, 2024

Thanks @crepererum and @jcsherin for the reviews

@alamb alamb deleted the alamb/remove_built_in_window branch November 15, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcsherin jcsherin jcsherin left review comments

@crepererum crepererum crepererum approved these changes

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions physical-expr Physical Expressions proto Related to proto crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alamb @jonathanc-n @jcsherin @crepererum