lustefaniak commented on PR #1435: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1435#issuecomment-2430336693
Hi @Nyrox! Thanks for this PR and pushing locations support further! We had an internal discussion to try to upstream the location changes we use in SYNQ. I found some time to finish rebase today and then noticed your PR. (https://github.com/apache/datafusion-sqlparser-rs/pull/1480) I like your suggested changes, as they are very noninvasive for the downstream consumers. If this could be included in the main, that would be absolutely amazing. Having location inside every Ident is probably best. In my PR, I wrapped all Idents with location, but our fork uses them only when we need them. Having it always available inside of `Indent` sounds much better. It would also be good to design and include wrapping of nodes. It is essential, especially for enum cases where adding location to each of the enum cases would be too verbose. Also in some places those "smaller" locations just don't make much sense. In my PR, I'm using https://github.com/apache/datafusion-sqlparser-rs/pull/1480/commits/fc8e44afd66c3a5100a6546d8edba36c65870315#diff-fc6a8d66b8cb6bd48a119a70e489cbcef82e7f551e7cf08e8e972ef4e774ef49R12259-R12308 to align captured span to a non-whitespace portion of the query based on token indexes. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
