choplin commented on code in PR #23063:
URL: https://github.com/apache/datafusion/pull/23063#discussion_r3733346997
##########
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:
@kosiew Sorry, I've noticed that fix I reported in 879faea doesn't work.
Validating in `SqlToRel` happens before the `TypeCoercion` analyzer rule, so
it only sees the argument types as written. This rejects valid queries. The
existing test `select_where_nullif_division` fails:
```sql
SELECT c3/(c4+c5) FROM aggregate_test_100 WHERE c3/nullif(c4+c5, 0) > 0.1
```
```
invalid argument type(s) for 'nullif'
called with argument type(s): Int32, Int64
candidate function(s): nullif(Int32, Int32)
```
`nullif(Int32, Int64)` is coercible to `nullif(Int32, Int32)`, but the check
runs too early to see the coerced types.
The difficulty is that the error is only known where coercion runs
(`verify_function_arguments`, during analysis), while the span is only known
during planning. Something has to bring the two together. These are the four
options I can think of:
1. Make the check in `SqlToRel` coercion-aware, so it accepts arguments that
coercion would accept. This keeps the current design and needs no struct
change, but it reimplements what `TypeCoercion` already does, in the planner,
just to produce a diagnostic. The two would have to stay in sync, and if they
drift the planner rejects queries the engine can run.
2. Point the diagnostic at the arguments. Arguments are `Expr::Column`s and
already carry spans, so `verify_function_arguments` can take the span from them
instead of from the call. Validation stays where it is, so valid queries are no
longer rejected. The location becomes approximate: `sum(first_name)` is
reported at `first_name`, and for a call like `sum(a + b)` it is reported at
whichever column comes first inside the argument.
3. Attach the span after analysis. The diagnostic is produced without a
span, and a later step matches it against the parsed statement by function name
to fill in the location. This changes no struct, but the match is a heuristic.
For `SELECT sum(a), sum(b)` there are two calls to the same function and no
reliable way to tell which one failed.
4. Add `spans` to the expression structs, as the original approach did, and
land it in a future major release.
Option 4 is the one I would prefer. If the expression itself holds the span,
the span is still available when the error is raised, so the check can stay in
`verify_function_arguments` and run after coercion. The diagnostic then points
at the call itself. It also follows `Column`, which has carried `pub spans:
Spans` since #13664.
If there is a better way to carry the span into analysis that I have missed,
I would be glad to hear it.
Your concern was about shipping the new public fields in a patch release. I
read the API health policy, but I could not tell whether a contribution that
has to wait for a major version bump is acceptable, or how such a change is
usually handled here. Could you advise? Depending on that, I can hold this PR
until the next major release, or land the diagnostic without a span now and add
the span later.
--
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]
