Skip to content

Commit c8a4431

Browse files
committed
feat: carry source span on WindowFunction for diagnostics
Window aggregate / window UDF calls (e.g. `sum(first_name) OVER ()`) previously passed `None` as the span into `verify_function_arguments`, so the resulting diagnostic could not point at the SQL call site. Add a `spans` field to `WindowFunction` (mirroring `ScalarFunction` / `AggregateFunction`), populate it in `SqlToRel`, and thread it through `ExprSchemable` so window function argument diagnostics carry the span. Add a regression test for the window function path. Addresses PR review feedback.
1 parent 9e33404 commit c8a4431

15 files changed

Lines changed: 81 additions & 15 deletions

File tree

‎datafusion-examples/examples/udf/advanced_udwf.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ impl WindowUDFImpl for SimplifySmoothItUdf {
201201
distinct: window_function.params.distinct,
202202
filter: window_function.params.filter,
203203
},
204+
spans: Default::default(),
204205
}))
205206
};
206207

‎datafusion/core/src/physical_planner.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2439,6 +2439,7 @@ pub fn create_window_expr_with_name(
24392439
distinct,
24402440
filter,
24412441
},
2442+
..
24422443
} = window_fun.as_ref();
24432444
let physical_args =
24442445
create_physical_exprs(args, logical_schema, execution_props)?;

‎datafusion/expr/src/expr.rs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1245,6 +1245,8 @@ pub struct WindowFunction {
12451245
/// Name of the function
12461246
pub fun: WindowFunctionDefinition,
12471247
pub params: WindowFunctionParams,
1248+
/// Original source code location, if known
1249+
pub spans: Spans,
12481250
}
12491251

12501252
#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
@@ -1280,9 +1282,15 @@ impl WindowFunction {
12801282
null_treatment: None,
12811283
distinct: false,
12821284
},
1285+
spans: Spans::new(),
12831286
}
12841287
}
12851288

1289+
/// Returns a mutable reference to the spans
1290+
pub fn spans_mut(&mut self) -> &mut Spans {
1291+
&mut self.spans
1292+
}
1293+
12861294
/// Returns this window function's simplification hook, if any.
12871295
///
12881296
/// See [`WindowFunctionSimplification`] for more information
@@ -2308,6 +2316,7 @@ impl Expr {
23082316
Expr::Not(inner) | Expr::Negative(inner) => inner.spans(),
23092317
Expr::ScalarFunction(func) => Some(&func.spans),
23102318
Expr::AggregateFunction(func) => Some(&func.spans),
2319+
Expr::WindowFunction(func) => Some(&func.spans),
23112320
_ => None,
23122321
}
23132322
}
@@ -2582,6 +2591,7 @@ impl NormalizeEq for Expr {
25822591
null_treatment: self_null_treatment,
25832592
distinct: self_distinct,
25842593
},
2594+
..
25852595
} = left.as_ref();
25862596
let WindowFunction {
25872597
fun: other_fun,
@@ -2595,6 +2605,7 @@ impl NormalizeEq for Expr {
25952605
null_treatment: other_null_treatment,
25962606
distinct: other_distinct,
25972607
},
2608+
..
25982609
} = other.as_ref();
25992610

26002611
self_fun.name() == other_fun.name()
@@ -2856,6 +2867,7 @@ impl HashNode for Expr {
28562867
null_treatment,
28572868
distinct,
28582869
},
2870+
..
28592871
} = window_fun.as_ref();
28602872
fun.hash(state);
28612873
window_frame.hash(state);
@@ -3175,7 +3187,7 @@ impl Display for SchemaDisplay<'_> {
31753187
Ok(())
31763188
}
31773189
Expr::WindowFunction(window_fun) => {
3178-
let WindowFunction { fun, params } = window_fun.as_ref();
3190+
let WindowFunction { fun, params, .. } = window_fun.as_ref();
31793191
match fun {
31803192
WindowFunctionDefinition::AggregateUDF(fun) => {
31813193
match fun.window_function_schema_name(params) {
@@ -3615,7 +3627,7 @@ impl Display for Expr {
36153627
fmt_function(f, fun.name(), false, &fun.args, true)
36163628
}
36173629
Expr::WindowFunction(window_fun) => {
3618-
let WindowFunction { fun, params } = window_fun.as_ref();
3630+
let WindowFunction { fun, params, .. } = window_fun.as_ref();
36193631
match fun {
36203632
WindowFunctionDefinition::AggregateUDF(fun) => {
36213633
match fun.window_function_display_name(params) {

‎datafusion/expr/src/expr_schema.rs‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ impl ExprSchemable for Expr {
539539
let WindowFunction {
540540
fun,
541541
params: WindowFunctionParams { args, .. },
542-
..
542+
spans,
543543
} = window_function.as_ref();
544544

545545
let fields = args
@@ -548,14 +548,20 @@ impl ExprSchemable for Expr {
548548
.collect::<Result<Vec<_>>>()?;
549549
match fun {
550550
WindowFunctionDefinition::AggregateUDF(udaf) => {
551-
let new_fields =
552-
verify_function_arguments(udaf.as_ref(), &fields, None)?;
551+
let new_fields = verify_function_arguments(
552+
udaf.as_ref(),
553+
&fields,
554+
spans.first(),
555+
)?;
553556
let return_field = udaf.return_field(&new_fields)?;
554557
Ok(return_field)
555558
}
556559
WindowFunctionDefinition::WindowUDF(udwf) => {
557-
let new_fields =
558-
verify_function_arguments(udwf.as_ref(), &fields, None)?;
560+
let new_fields = verify_function_arguments(
561+
udwf.as_ref(),
562+
&fields,
563+
spans.first(),
564+
)?;
559565
let return_field = udwf
560566
.field(WindowUDFFieldArgs::new(&new_fields, &schema_name))?;
561567
Ok(return_field)

‎datafusion/expr/src/logical_plan/plan.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2809,6 +2809,7 @@ impl Window {
28092809
let WindowFunction {
28102810
fun: WindowFunctionDefinition::WindowUDF(udwf),
28112811
params: WindowFunctionParams { partition_by, .. },
2812+
..
28122813
} = window_fun.as_ref()
28132814
else {
28142815
return None;

‎datafusion/expr/src/tree_node.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ impl TreeNode for Expr {
275275
null_treatment,
276276
distinct,
277277
},
278+
spans,
278279
} = *window_fun;
279280

280281
(args, partition_by, order_by, filter)
@@ -292,6 +293,7 @@ impl TreeNode for Expr {
292293
null_treatment,
293294
distinct,
294295
},
296+
spans,
295297
}))
296298
},
297299
)?

‎datafusion/functions-window/src/planner.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
//! SQL planning extensions like [`WindowFunctionPlanner`]
1919
20-
use datafusion_common::Result;
20+
use datafusion_common::{Result, Spans};
2121
use datafusion_expr::{
2222
Expr,
2323
expr::{WindowFunction, WindowFunctionParams},
@@ -56,6 +56,7 @@ impl ExprPlanner for WindowFunctionPlanner {
5656
null_treatment,
5757
distinct,
5858
},
59+
spans: Spans::new(),
5960
});
6061

6162
let saved_name = NamePreserver::new_for_projection().save(&origin_expr);
@@ -75,6 +76,7 @@ impl ExprPlanner for WindowFunctionPlanner {
7576
distinct,
7677
filter,
7778
},
79+
..
7880
} = *window_fun;
7981
let raw_expr = RawWindowExpr {
8082
func_def: fun,
@@ -116,6 +118,7 @@ impl ExprPlanner for WindowFunctionPlanner {
116118
null_treatment,
117119
distinct,
118120
},
121+
spans: Spans::new(),
119122
});
120123
let new_expr = saved_name.restore(new_expr);
121124

‎datafusion/optimizer/src/analyzer/type_coercion.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@ use arrow::temporal_conversions::SECONDS_IN_DAY;
3030
use datafusion_common::config::ConfigOptions;
3131
use datafusion_common::tree_node::{Transformed, TreeNode, TreeNodeRewriter};
3232
use datafusion_common::{
33-
Column, DFSchema, DFSchemaRef, DataFusionError, Result, ScalarValue, TableReference,
34-
exec_err, internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err,
35-
plan_err,
33+
Column, DFSchema, DFSchemaRef, DataFusionError, Result, ScalarValue, Spans,
34+
TableReference, exec_err, internal_datafusion_err, internal_err, not_impl_err,
35+
plan_datafusion_err, plan_err,
3636
};
3737
use datafusion_expr::expr::{
3838
self, AggregateFunctionParams, Alias, Between, BinaryExpr, Case, Exists,
@@ -746,6 +746,7 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
746746
null_treatment,
747747
distinct,
748748
},
749+
..
749750
} = *window_fun;
750751
let window_frame =
751752
coerce_window_frame(window_frame, self.schema, &order_by)?;
@@ -775,6 +776,7 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
775776
null_treatment,
776777
distinct,
777778
},
779+
spans: Spans::new(),
778780
});
779781
Ok(Transformed::yes(new_expr))
780782
}

‎datafusion/proto/src/logical_plan/to_proto.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,7 @@ pub fn serialize_expr(
324324
distinct,
325325
filter,
326326
},
327+
..
327328
} = window_fun.as_ref();
328329
let mut buf = Vec::new();
329330
let window_function = match fun {

‎datafusion/sql/src/expr/function.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
1919

2020
use arrow::datatypes::{DataType, FieldRef};
2121
use datafusion_common::{
22-
DFSchema, Dependency, Diagnostic, HashSet, Result, Span, datatype::FieldExt,
22+
DFSchema, Dependency, Diagnostic, HashSet, Result, Span, Spans, datatype::FieldExt,
2323
internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err,
2424
};
2525
use datafusion_expr::{
@@ -682,7 +682,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
682682
distinct,
683683
} = window_expr;
684684

685-
let inner = WindowFunction {
685+
let mut inner = WindowFunction {
686686
fun: func_def,
687687
params: expr::WindowFunctionParams {
688688
args,
@@ -693,7 +693,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
693693
null_treatment,
694694
distinct,
695695
},
696+
spans: Spans::new(),
696697
};
698+
if self.options.collect_spans
699+
&& let Some(span) = Span::try_from_sqlparser_span(sql_parser_span)
700+
{
701+
inner.spans_mut().add_span(span);
702+
}
697703

698704
if name.eq_ignore_ascii_case(inner.fun.name()) {
699705
return Ok(Expr::WindowFunction(Box::new(inner)));

0 commit comments

Comments
 (0)