Skip to content

feat: attach Diagnostic to "invalid function argument types" error #23063

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

Open
choplin wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: attach Diagnostic to "invalid function argument types" error #23063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
347c00a
feat: attach Diagnostic to "invalid function argument types" error
choplin Jun 14, 2026
8fc9954
chore(sql): fix formatting in diagnostic tests
choplin Jun 25, 2026
9e33404
fix(substrait): mention new `spans` field in AggregateFunction pattern
choplin Jul 1, 2026
c8a4431
feat: carry source span on WindowFunction for diagnostics
choplin Jul 8, 2026
371b705
test: add scalar-function argument-type diagnostic regression test
choplin Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datafusion-examples/examples/udf/advanced_udwf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ impl WindowUDFImpl for SimplifySmoothItUdf {
distinct: window_function.params.distinct,
filter: window_function.params.filter,
},
spans: Default::default(),
}))
};

Expand Down
1 change: 1 addition & 0 deletions datafusion/core/src/physical_planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2439,6 +2439,7 @@ pub fn create_window_expr_with_name(
distinct,
filter,
},
..
} = window_fun.as_ref();
let physical_args =
create_physical_exprs(args, logical_schema, execution_props)?;
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/tests/execution/logical_plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ async fn count_only_nulls() -> Result<()> {
order_by: vec![],
null_treatment: None,
},
spans: Spans::new(),
})],
)?);

Expand Down
57 changes: 49 additions & 8 deletions datafusion/expr/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,21 +930,32 @@ pub struct ScalarFunction {
pub func: Arc<crate::ScalarUDF>,
/// List of expressions to feed to the functions as arguments
pub args: Vec<Expr>,
/// Original source code location, if known
pub spans: Spans,

@kosiew kosiew Jul 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we avoid adding spans as a required public field on these expression structs?

ScalarFunction, AggregateFunction, and WindowFunction are publicly constructible with struct literals. Adding a new required field means existing downstream code using those literals will no longer compile. cargo-semver-checks reports constructible_struct_adds_field for ScalarFunction.spans, AggregateFunction.spans, and WindowFunction.spans.

Please keep the diagnostic span data outside these public struct layouts, or use another semver-compatible design. The new required public fields should be removed before this is merged.

@choplin choplin Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry for the slow reply here!

You're right about the semver break — adding spans to these public structs trips
constructible_struct_adds_field.

My thinking so far: I noticed Column already carries the same pub spans: Spans
field, added the same way to that (publicly constructible) struct in the earlier
diagnostics work (#13664) — so one direction would be to stay consistent with that
precedent. Another would be to keep the span off these struct layouts entirely and
validate arguments eagerly in SqlToRel, where the call-site span is already in scope,
so the diagnostic is attached at planning time.

I'm not sure which fits best here, though, and there may well be a cleaner
semver-compatible approach I'm not seeing. Do you have a direction you'd prefer — or a
better idea than either of these?

@kosiew kosiew Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Between the two options you mentioned, I'd lean toward keeping the parser-only span information out of these public struct layouts. Validating in SqlToRel, where the call-site span is already available, seems like a good fit and avoids introducing a semver break while still preserving the improved diagnostics. If there's another semver-compatible way to associate the span with these expressions, that would also work, but I don't think adding the new public fields is something we should ship in a patch release.

}

impl ScalarFunction {
// return the Function's name
pub fn name(&self) -> &str {
self.func.name()
}

/// Returns a mutable reference to the spans
pub fn spans_mut(&mut self) -> &mut Spans {
&mut self.spans
}
}

impl ScalarFunction {
/// Create a new `ScalarFunction` from a [`ScalarUDF`]
///
/// [`ScalarUDF`]: crate::ScalarUDF
pub fn new_udf(udf: Arc<crate::ScalarUDF>, args: Vec<Expr>) -> Self {
Self { func: udf, args }
Self {
func: udf,
args,
spans: Spans::new(),
}
}
}

Expand Down Expand Up @@ -1094,6 +1105,8 @@ pub struct AggregateFunction {
/// Name of the function
pub func: Arc<AggregateUDF>,
pub params: AggregateFunctionParams,
/// Original source code location, if known
pub spans: Spans,
}

#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
Expand Down Expand Up @@ -1127,8 +1140,14 @@ impl AggregateFunction {
order_by,
null_treatment,
},
spans: Spans::new(),
}
}

/// Returns a mutable reference to the spans
pub fn spans_mut(&mut self) -> &mut Spans {
&mut self.spans
}
}

/// A function used as a SQL window function
Expand Down Expand Up @@ -1226,6 +1245,8 @@ pub struct WindowFunction {
/// Name of the function
pub fun: WindowFunctionDefinition,
pub params: WindowFunctionParams,
/// Original source code location, if known
pub spans: Spans,
}

#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
Expand Down Expand Up @@ -1261,9 +1282,15 @@ impl WindowFunction {
null_treatment: None,
distinct: false,
},
spans: Spans::new(),
}
}

/// Returns a mutable reference to the spans
pub fn spans_mut(&mut self) -> &mut Spans {
&mut self.spans
}

/// Returns this window function's simplification hook, if any.
///
/// See [`WindowFunctionSimplification`] for more information
Expand Down Expand Up @@ -2287,6 +2314,9 @@ impl Expr {
match self {
Expr::Column(col) => Some(&col.spans),
Expr::Not(inner) | Expr::Negative(inner) => inner.spans(),
Expr::ScalarFunction(func) => Some(&func.spans),
Expr::AggregateFunction(func) => Some(&func.spans),
Expr::WindowFunction(func) => Some(&func.spans),
_ => None,
}
}
Expand Down Expand Up @@ -2482,10 +2512,12 @@ impl NormalizeEq for Expr {
Expr::ScalarFunction(ScalarFunction {
func: self_func,
args: self_args,
..
}),
Expr::ScalarFunction(ScalarFunction {
func: other_func,
args: other_args,
..
}),
) => {
self_func.name() == other_func.name()
Expand All @@ -2506,6 +2538,7 @@ impl NormalizeEq for Expr {
order_by: self_order_by,
null_treatment: self_null_treatment,
},
..
}),
Expr::AggregateFunction(AggregateFunction {
func: other_func,
Expand All @@ -2517,6 +2550,7 @@ impl NormalizeEq for Expr {
order_by: other_order_by,
null_treatment: other_null_treatment,
},
..
}),
) => {
self_func.name() == other_func.name()
Expand Down Expand Up @@ -2557,6 +2591,7 @@ impl NormalizeEq for Expr {
null_treatment: self_null_treatment,
distinct: self_distinct,
},
..
} = left.as_ref();
let WindowFunction {
fun: other_fun,
Expand All @@ -2570,6 +2605,7 @@ impl NormalizeEq for Expr {
null_treatment: other_null_treatment,
distinct: other_distinct,
},
..
} = other.as_ref();

self_fun.name() == other_fun.name()
Expand Down Expand Up @@ -2797,7 +2833,9 @@ impl HashNode for Expr {
| Expr::TryCast(TryCast { expr: _expr, field }) => {
field.hash(state);
}
Expr::ScalarFunction(ScalarFunction { func, args: _args }) => {
Expr::ScalarFunction(ScalarFunction {
func, args: _args, ..
}) => {
func.hash(state);
}
Expr::AggregateFunction(AggregateFunction {
Expand All @@ -2810,6 +2848,7 @@ impl HashNode for Expr {
order_by: _,
null_treatment,
},
..
}) => {
func.hash(state);
distinct.hash(state);
Expand All @@ -2828,6 +2867,7 @@ impl HashNode for Expr {
null_treatment,
distinct,
},
..
} = window_fun.as_ref();
fun.hash(state);
window_frame.hash(state);
Expand Down Expand Up @@ -2952,7 +2992,7 @@ impl Display for SchemaDisplay<'_> {
| Expr::OuterReferenceColumn(..)
| Expr::Placeholder(_)
| Expr::Wildcard { .. } => write!(f, "{}", self.0),
Expr::AggregateFunction(AggregateFunction { func, params }) => {
Expr::AggregateFunction(AggregateFunction { func, params, .. }) => {
match func.schema_name(params) {
Ok(name) => {
write!(f, "{name}")
Expand Down Expand Up @@ -3109,7 +3149,7 @@ impl Display for SchemaDisplay<'_> {
Expr::Unnest(Unnest { expr }) => {
write!(f, "UNNEST({})", SchemaDisplay(expr))
}
Expr::ScalarFunction(ScalarFunction { func, args }) => {
Expr::ScalarFunction(ScalarFunction { func, args, .. }) => {
match func.schema_name(args) {
Ok(name) => {
write!(f, "{name}")
Expand Down Expand Up @@ -3147,7 +3187,7 @@ impl Display for SchemaDisplay<'_> {
Ok(())
}
Expr::WindowFunction(window_fun) => {
let WindowFunction { fun, params } = window_fun.as_ref();
let WindowFunction { fun, params, .. } = window_fun.as_ref();
match fun {
WindowFunctionDefinition::AggregateUDF(fun) => {
match fun.window_function_schema_name(params) {
Expand Down Expand Up @@ -3408,7 +3448,7 @@ impl Display for SqlDisplay<'_> {

Ok(())
}
Expr::AggregateFunction(AggregateFunction { func, params }) => {
Expr::AggregateFunction(AggregateFunction { func, params, .. }) => {
match func.human_display(params) {
Ok(name) => {
write!(f, "{name}")
Expand Down Expand Up @@ -3587,7 +3627,7 @@ impl Display for Expr {
fmt_function(f, fun.name(), false, &fun.args, true)
}
Expr::WindowFunction(window_fun) => {
let WindowFunction { fun, params } = window_fun.as_ref();
let WindowFunction { fun, params, .. } = window_fun.as_ref();
match fun {
WindowFunctionDefinition::AggregateUDF(fun) => {
match fun.window_function_display_name(params) {
Expand Down Expand Up @@ -3639,7 +3679,7 @@ impl Display for Expr {
}
}
}
Expr::AggregateFunction(AggregateFunction { func, params }) => {
Expr::AggregateFunction(AggregateFunction { func, params, .. }) => {
match func.display_name(params) {
Ok(name) => {
write!(f, "{name}")
Expand Down Expand Up @@ -4399,6 +4439,7 @@ mod test {
let udf = Expr::ScalarFunction(ScalarFunction {
func: Arc::new(ScalarUDF::new_from_impl(TestUDF {})),
args: vec![expr(), expr()],
spans: Spans::new(),
});
let Expr::ScalarFunction(scalar) = &udf else {
unreachable!()
Expand Down
72 changes: 51 additions & 21 deletions datafusion/expr/src/expr_schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ use arrow::datatypes::FieldRef;
use arrow::datatypes::{DataType, Field};
use datafusion_common::datatype::FieldExt;
use datafusion_common::{
Column, DataFusionError, ExprSchema, Result, ScalarValue, Spans, TableReference,
not_impl_err, plan_datafusion_err, plan_err,
Column, DataFusionError, Diagnostic, ExprSchema, Result, ScalarValue, Span, Spans,
TableReference, not_impl_err, plan_datafusion_err, plan_err,
};
use datafusion_expr_common::type_coercion::binary::BinaryTypeCoercer;
use datafusion_functions_window_common::field::WindowUDFFieldArgs;
Expand Down Expand Up @@ -539,7 +539,7 @@ impl ExprSchemable for Expr {
let WindowFunction {
fun,
params: WindowFunctionParams { args, .. },
..
spans,
} = window_function.as_ref();

let fields = args
Expand All @@ -548,14 +548,20 @@ impl ExprSchemable for Expr {
.collect::<Result<Vec<_>>>()?;
match fun {
WindowFunctionDefinition::AggregateUDF(udaf) => {
let new_fields =
verify_function_arguments(udaf.as_ref(), &fields)?;
let new_fields = verify_function_arguments(
udaf.as_ref(),
&fields,
spans.first(),
)?;
let return_field = udaf.return_field(&new_fields)?;
Ok(return_field)
}
WindowFunctionDefinition::WindowUDF(udwf) => {
let new_fields =
verify_function_arguments(udwf.as_ref(), &fields)?;
let new_fields = verify_function_arguments(
udwf.as_ref(),
&fields,
spans.first(),
)?;
let return_field = udwf
.field(WindowUDFFieldArgs::new(&new_fields, &schema_name))?;
Ok(return_field)
Expand All @@ -565,20 +571,23 @@ impl ExprSchemable for Expr {
Expr::AggregateFunction(AggregateFunction {
func,
params: AggregateFunctionParams { args, .. },
spans,
}) => {
let fields = args
.iter()
.map(|e| e.to_field(schema).map(|(_, f)| f))
.collect::<Result<Vec<_>>>()?;
let new_fields = verify_function_arguments(func.as_ref(), &fields)?;
let new_fields =
verify_function_arguments(func.as_ref(), &fields, spans.first())?;
func.return_field(&new_fields)
}
Expr::ScalarFunction(ScalarFunction { func, args }) => {
Expr::ScalarFunction(ScalarFunction { func, args, spans }) => {
let fields = args
.iter()
.map(|e| e.to_field(schema).map(|(_, f)| f))
.collect::<Result<Vec<_>>>()?;
let new_fields = verify_function_arguments(func.as_ref(), &fields)?;
let new_fields =
verify_function_arguments(func.as_ref(), &fields, spans.first())?;

let arguments = args
.iter()
Expand Down Expand Up @@ -720,25 +729,46 @@ impl ExprSchemable for Expr {
fn verify_function_arguments<F: UDFCoercionExt>(
function: &F,
input_fields: &[FieldRef],
func_span: Option<Span>,
) -> Result<Vec<FieldRef>> {
fields_with_udf(input_fields, function).map_err(|err| {
let data_types = input_fields
.iter()
.map(|f| f.data_type())
.cloned()
.collect::<Vec<_>>();
plan_datafusion_err!(
"{}. {}",
match err {
DataFusionError::Plan(msg) => msg,
err => err.to_string(),
},
utils::generate_signature_error_message(
function.name(),
function.signature(),
&data_types
)
let name = function.name();
let signature_msg = utils::generate_signature_error_message(
name,
function.signature(),
&data_types,
);
let err_msg = match err {
DataFusionError::Plan(msg) => msg,
err => err.to_string(),
};

let types_str = data_types
.iter()
.map(|dt| dt.to_string())
.collect::<Vec<_>>()
.join(", ");
let candidates = function
.signature()
.type_signature
.to_string_repr_with_names(function.signature().parameter_names.as_deref())
.iter()
.map(|args_str| format!("{name}({args_str})"))
.collect::<Vec<_>>()
.join(", ");
let diagnostic = Diagnostic::new_error(
format!("invalid argument type(s) for '{name}'"),
func_span,
)
.with_note(format!("called with argument type(s): {types_str}"), None)
.with_help(format!("candidate function(s): {candidates}"), None);

plan_datafusion_err!("{err_msg}. {signature_msg}").with_diagnostic(diagnostic)
})
}

Expand Down
1 change: 1 addition & 0 deletions datafusion/expr/src/logical_plan/plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2809,6 +2809,7 @@ impl Window {
let WindowFunction {
fun: WindowFunctionDefinition::WindowUDF(udwf),
params: WindowFunctionParams { partition_by, .. },
..
} = window_fun.as_ref()
else {
return None;
Expand Down
Loading
Loading