choplin commented on code in PR #23063:
URL: https://github.com/apache/datafusion/pull/23063#discussion_r3623581941
##########
datafusion/expr/src/expr.rs:
##########
@@ -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,
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
