Rename / simplify BuiltInWindowExpr / BuiltInWindowFunctionExpr

Is your feature request related to a problem or challenge?

We have completed converting all window functions to use the user defined API (specifically WindowFunctionImpl). See [Epic] Unify WindowFunction Interface (remove built in list of BuiltInWindowFunction s) #8709

However, there are still a few structures named BuiltInWindow* that now no longer reflect the reality that there are no more "built in" functions:

A BuiltInWindowExpr here:

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

Lines 38 to 45 in 22c1f54

    
           /// A window expr that takes the form of a [`BuiltInWindowFunctionExpr`]. 
        
           #[derive(Debug)] 
        
           pub struct BuiltInWindowExpr { 
        
               expr: Arc<dyn BuiltInWindowFunctionExpr>, 
        
               partition_by: Vec<Arc<dyn PhysicalExpr>>, 
        
               order_by: LexOrdering, 
        
               window_frame: Arc<WindowFrame>, 
        
           }

A BuiltInWindowFunctionExpr:

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

Lines 30 to 39 in 22c1f54

    
           /// Evaluates a window function by instantiating a 
        
           /// `[PartitionEvaluator]` for calculating the function's output in 
        
           /// that partition. 
        
           /// 
        
           /// Note that unlike aggregation based window functions, some window 
        
           /// functions such as `rank` ignore the values in the window frame, 
        
           /// but others such as `first_value`, `last_value`, and 
        
           /// `nth_value` need the value. 
        
           #[allow(rustdoc::private_intra_doc_links)] 
        
           pub trait BuiltInWindowFunctionExpr: Send + Sync + std::fmt::Debug {

Which confusingly is implemented for WindowUDFExpr:

datafusion/datafusion/physical-plan/src/windows/mod.rs

Lines 218 to 221 in d840e98

    
           impl BuiltInWindowFunctionExpr for WindowUDFExpr { 
        
               fn as_any(&self) -> &dyn std::any::Any { 
        
                   self 
        
               }

Describe the solution you'd like

I would like to rename the confusing BuiltIn prefix for these structures
Leave backwards compatibility typedefs type BuiltInWindowExpr = ... to ease migration

Describe alternatives you've considered

I would recommend first simply renaming the structs / traits to remove the BuiltIn prefix / module names

Then I also think it is worth seeing if the BuiltInWindowFunctionExpr trait is still needed, or if the code in physical plan could simply call WindowUDF directly

Additional context

No response

take

@irenjj

Thanks to @irenjj I think this one is more or less done, so closing the ticket

Navigation Menu

Repository navigation

Rename / simplify `BuiltInWindowExpr` / `BuiltInWindowFunctionExpr` #13473

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/// A window expr that takes the form of a [`BuiltInWindowFunctionExpr`].
	#[derive(Debug)]
	pub struct BuiltInWindowExpr {
	expr: Arc<dyn BuiltInWindowFunctionExpr>,
	partition_by: Vec<Arc<dyn PhysicalExpr>>,
	order_by: LexOrdering,
	window_frame: Arc<WindowFrame>,
	}

	/// Evaluates a window function by instantiating a
	/// `[PartitionEvaluator]` for calculating the function's output in
	/// that partition.
	///
	/// Note that unlike aggregation based window functions, some window
	/// functions such as `rank` ignore the values in the window frame,
	/// but others such as `first_value`, `last_value`, and
	/// `nth_value` need the value.
	#[allow(rustdoc::private_intra_doc_links)]
	pub trait BuiltInWindowFunctionExpr: Send + Sync + std::fmt::Debug {

	impl BuiltInWindowFunctionExpr for WindowUDFExpr {
	fn as_any(&self) -> &dyn std::any::Any {
	self
	}

Rename / simplify BuiltInWindowExpr / BuiltInWindowFunctionExpr #13473

Description

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Activity

irenjj commented on Nov 19, 2024

alamb commented on Dec 21, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Rename / simplify `BuiltInWindowExpr` / `BuiltInWindowFunctionExpr` #13473