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 here1
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 in #13664 (the parent Diagnostics EPIC this PR
belongs to),
and it's likewise publicly constructible — 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?
##########
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 in #13664 (the parent Diagnostics EPIC this PR
belongs to),
and it's likewise publicly constructible — 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]
