Add Location/Spans in to the AST/parse layer

[jakeswenson]

This is an attempt at starting to plumb a location (or in this case a Span) through to the parsing/AST layer: #757.

Changes

1. Add a new wrapper type WithSpan<T> which can be used to wrap and attach a span to any AST node. Not sure about this approach yet, but it seemed to work OK. I think this might need to evolve in to a type called WithTrivia<T> where Span and "trivia items" like leading and trailing Token's to an AST node are attached. (i.e. the SELECT keyword for a query, to get a span for the entire select query.) This "trivia" based approach is something I've learned from dotnet/roslyn (the C# compiler). (This trivia based approach was also taken with typescript.)
2. Add a Span type which is a struct with a start (inclusive) and end (exclusive) Location

ToDo still

• Finish wiring spans through the tests. This is going to be the most annoying part about this work. Would love to hear folks thoughts on this.

Open Questions

• Should this work just align to using zkat/miette Diagnostic and SourceSpan.

[ankrgyl]

@jakeswenson thanks for putting this up! Can you talk about how it's different than the approach in #790?

The key problem we're trying to work through right now is how to avoid breaking downstream users of sqlparser by introducing locations.

[jakeswenson]

I've fixed this previous possible panic! in the parser for these GRANT statement.

[jakeswenson]

another panic! fix in REVOKE

[alamb]

cc @ankrgyl who I believe was doing something similar

[ankrgyl]

@alamb @jakeswenson yes indeed! I posted this above too, but it'd be great to understand the differences between this and #790. In general, I'd prefer to collaborate on that PR vs. work on two separate ones.

[lustefaniak]

I used this PR in our forked https://github.com/getsynq/sqlparser-rs and a tool built on top of that, and it works amazingly well. Adding empty_span() all over the place is painful initially, but in the end, all works as it should and is not much of a burden.

Since I don't have much Rust experience, I can't comment on any technical downsides(if any) of that design, but the "client" code required minimal changes to make it fully work.

[alamb]

This is the commit in case anyone is interested: getsynq@b4547ca

Since this feature keeps being requested, it would be great to get something ready to go

[lustefaniak]

After working on column-level lineage using sqlparser-rs even more, I think we must add location to every instance of Ident and maybe also ObjectName as otherwise it is super annoying to mark every Ident as WithSpan<Ident> and then patching all the tests 🤔

[yuyang-ok]

Is this going to merge or not?

[alamb]

Is this going to merge or not?

I don't think this PR is mergeable as is (it has conflicts and doesn't pass CI)

As the Location/Span thing keeps coming up it would be awesome for someone to take a stab at implementing it. The core challenge, in my mind, of adding this feature is minimizing downstream crate impacts (sqlparser is used quite widely). I don't think we'll be able to avoid all impacts but being able to minimize the impact is important

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.