Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surface location in parse errors #1046

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scsmithr
Copy link

@scsmithr scsmithr commented Nov 16, 2023

Surfaces location in ParserError to allow library users to be able to enrich error messages that are shown to end users. The annotate_with_error shows an example use case where we can generate an error string that shows the full query with the error displayed inline.

For example, if I submit the following query:

select
    a,
    b,,
from public.letters
order by b

I can take the error from parsing that, and use the extra location info to be able to generate the following:

select
    a,
    b,,
      ^ Expected an expression:, found: ,
from public.letters
order by b

This is a breaking change as it's touching ParserError, and downstream projects (datafusion) would be impacted by this (the custom sql parser in particular). I added in ParserError::new_parser_error to make the changes need relatively straightforward.

Longer term, I think ParserError can be improved a bit more to allow even better diagnostics to be displayed to the end user. What I'm thinking:

pub struct ParserError<'a> {
    /// The kind of the error, parsing, tokenenizing, recursion limit, etc.
    pub kind: ParserErrorKind,

    /// Error message to show the user.
    pub msg: String,

    /// The query currently being parsed.
    pub query: &'a str,

    /// Location context (or a span if that gets threaded around).
    pub location: Option<Location>,
}

Obviously that's an even more breaking change, but I think it'd be worth it to make the errors returned to users very informative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant