apache / apache/datafusion

[EPIC] Attach `Diagnostic` to more errors

Open
#14,429 7 comments 7 reactions 0 assignees View on GitHub
enhancement EPIC
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Is your feature request related to a problem or challenge?

In #13664 we introduced the `Diagnostic` type and `DataFusionError::Diagnostic`. They allow enriching errors with messages meant for consumption by end users of an application built on top of DataFusion, by providing rich information and context that directly references locations in the SQL query. They enable features like:

![](https://github.com/user-attachments/assets/32efeb4f-aad9-41c9-a1e3-600a00d525c3)

See `datafusion/sql/tests/cases/diagnostic` for examples on how to extract and use diagnostics:

https://github.com/apache/datafusion/blob/d5428b21d6486e5b7db525d314ee9119a512c397/datafusion/sql/tests/cases/diagnostic.rs#L132-L140

In that PR, we only implemented diagnostics for:

- Unresolved table references
- Unresolved column references (qualified and non)
- Non-aggregate expressions missing from `GROUP BY` clause
- Ambiguous column references
- Wrong number of columns in set expression (e.g. `UNION`)
- Incompatible types in binary expressions

This issue is about using `Diagnostic` in more places, and adding related tests to `datafusion/sql/tests/cases/diagnostic`. We think we should at least implement the following, but suggestions are welcome and encouraged:

- [x] #14430
- [ ] #14431
- [ ] #14432
- [x] #14433
- [x] #14434
- [ ] #14436
- [x] #14437
- [x] #14438
- [ ] https://github.com/apache/datafusion/issues/15276

### Describe the solution you'd like

The implementation should follow the steps of #13664, by calling `DataFusionError.with_diagnostic` to attach a `Diagnostic` to an error that is currently being returned. Tests should be added to `datafusion/sql/tests/cases/diagnostic` for each newly supported scenario.

For some of these items, it might be necessary to enrich the logical types with the `Span` information coming from the parser. This should be done using the `datafusion::common::Spans` type (note the "s"), introduced in #13664 to add span information to `datafusion::common::Column`.

It is desirable that the implementation is as little invasive as possible, in that it shouldn't require changing tons of function calls and types, unless absolutely necessary. The public facing API shouldn't change. The `Diagnostic` should be attached as soon as possible to the creation of the wrapped `DataFusionError` (i.e. deep in the call stack) and every error should ideally have just one `Diagnostic`.

### Describe alternatives you've considered

_No response_

### Additional context

_No response_

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the checked and unchecked subtasks linked from this epic, then read the existing examples in datafusion/sql/tests/cases/diagnostic and the Diagnostic implementation from #13664. The relevant work attaches diagnostics with DataFusionError.with_diagnostic, potentially using datafusion::common::Spans. Done means a selected error scenario has diagnostic coverage and related tests in datafusion/sql/tests/cases/diagnostic.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.