feat: attach Diagnostic to "invalid function argument types" error - #23063
feat: attach Diagnostic to "invalid function argument types" error#23063choplin wants to merge 5 commits into
Conversation
github-actions
Bot
added
sql
|
@choplin |
Add span-aware Diagnostic to the error produced by `verify_function_arguments` when a scalar or aggregate function is called with argument types that do not match any supported signature. - Add `spans: Spans` field to `ScalarFunction` and `AggregateFunction` so function-call source locations survive planning and optimization. - Populate spans from sqlparser in the SQL planner when `collect_spans` is enabled. - Pass the function-call span to `Diagnostic::new_error` so downstream consumers can highlight the offending call site. - Update all pattern matches, tree-node transforms, and serialization code for the new field. Closes apache#14431
Collapse a double blank line flagged by rustfmt (blank_lines_upper_bound) in the diagnostic test file.
0eeb855 to
8fc9954
Compare
|
@kosiew I rebased onto the latest main HEAD and resolved the conflict. |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
The `spans` field added to `expr::AggregateFunction` was not included in the substrait producer's exhaustive pattern match, breaking the build for the (non-default) substrait feature with E0027.
| WindowFunctionDefinition::AggregateUDF(udaf) => { | ||
| let new_fields = | ||
| verify_function_arguments(udaf.as_ref(), &fields)?; | ||
| verify_function_arguments(udaf.as_ref(), &fields, None)?; |
There was a problem hiding this comment.
Window aggregate calls still seem to produce diagnostics without a source span.
The regular aggregate path passes spans.first() into verify_function_arguments, but the Expr::WindowFunction path now passes None for both aggregate and window UDFs. For example, SELECT sum(first_name) OVER () FROM person reaches this branch through WindowFunctionDefinition::AggregateUDF, so the new invalid argument type(s) for 'sum' diagnostic cannot highlight the SQL call site.
Could you carry the function-call span on WindowFunction too, set it in SqlToRel when building the window expression, and pass it through here?
There was a problem hiding this comment.
Thanks! Fixed in c8a4431.
WindowFunction now carries a spans field (mirroring ScalarFunction / AggregateFunction). SqlToRel populates it from the function-call token when collect_spans is enabled, and both the AggregateUDF and WindowUDF window branches now pass spans.first() into verify_function_arguments instead of None, so sum(first_name) OVER () and window-UDF calls point at the SQL call site.
Added test_invalid_window_function_argument_types based on your example (sum(first_name) OVER ()), asserting the diagnostic points at the sum call site.
| Ok(()) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
It would be helpful to add a scalar-function regression test as well.
The PR covers scalar and aggregate calls, but the new test only exercises the aggregate path. A small invalid scalar call with collect_spans enabled would protect the other changed branch too.
There was a problem hiding this comment.
Done in 371b705 — added test_invalid_scalar_function_argument_types.
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.
The "invalid function argument types" work covered scalar and aggregate calls, but the diagnostic test suite only exercised the aggregate path. Add a test for an invalid scalar call (`power(first_name, first_name)`) with `collect_spans` enabled to cover the scalar branch. Addresses PR review feedback.
|
@kosiew Thanks for the review! I've addressed the two points from your review. Please take a look! |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the updates. The window aggregate and window UDF diagnostics now propagate WindowFunction.spans from SqlToRel and pass spans.first() into verify_function_arguments, so that concern is addressed. The scalar-function regression test has also been added.
There is still one high-severity semver issue that needs to be resolved before merge. The new spans fields change the public layouts of externally constructible expression structs.
| /// List of expressions to feed to the functions as arguments | ||
| pub args: Vec<Expr>, | ||
| /// Original source code location, if known | ||
| pub spans: Spans, |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| /// List of expressions to feed to the functions as arguments | ||
| pub args: Vec<Expr>, | ||
| /// Original source code location, if known | ||
| pub spans: Spans, |
There was a problem hiding this comment.
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.
2 participants
Which issue does this PR close?
Diagnosticto "invalid function argument types" error #14431Rationale for this change
When a scalar or aggregate function is called with argument types that don't match any supported signature (e.g.
SELECT sum('a')), the error message is a raw text dump with no source location. This PR attaches aDiagnosticso downstream consumers can highlight the offending call site in the original SQL query.What changes are included in this PR?
spans: Spansfield toScalarFunctionandAggregateFunctionso function-call source locations survive planning and optimization.collect_spansis enabled, following the same pattern used forColumn.verify_function_arguments, attach aDiagnosticwith:invalid argument type(s) for 'sum')called with argument type(s): Utf8)candidate function(s): sum(Decimal), sum(UInt64), ...)spansfield.Are there any user-facing changes?
No breaking API changes.
ScalarFunctionandAggregateFunctiongain aspans: Spansfield, butSpansis ignored inPartialEq/Hashand defaults to empty, so existing code usingScalarFunction::new_udf/AggregateFunction::new_udfis unaffected.When an error occurs, a diagnositc is attached in the error message.
Before (no
Diagnosticattached):After (same error message, with
Diagnosticnow attached):