You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "RPG-Alex (via GitHub)" <gi...@apache.org> on 2025/10/09 12:06:55 UTC
[I] Inline Comment Support for AST [datafusion-sqlparser-rs]
RPG-Alex opened a new issue, #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065
Limited Inline Comment Support for AST
I noticed this [previous issue](https://github.com/apache/datafusion-sqlparser-rs/issues/175), which is still open, and also found this this [PR](https://github.com/apache/datafusion-sqlparser-rs/issues/175) related to lossless AST.
It would be advantageous to include SQL inline comments in the AST, associated with the field they are for, which I think should be easily inferred for comments on tables and columns.
There is already support for `comment on`, and support for inline comments, as an option when parsing could be very helpful for workflows such as with generating documentation from comments.
--
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-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3566349938
ah, i'm sorry for being misleading :-/ i'll try to explain in more details:
in my proof-of-concept program i do have a `fn find_between(&self, start: Location, end: Location) -> impl Iterator<Item = &CommentWithSpan>` method (i just added it to the proposal above) allowing me to ask for comments between two locations. for example between the end location of one statement (ie. the statement's span's end) and the beginning of the next one (the statement's span's start.)
```
-- doc for statement one
select * from dual;
-- doc for statement two
select * from dual;
```
this works nicely since those locations (statements' spans) are being part of the AST (or are going to be there in soon future.) but some tokens are simply _not_ part of the AST (and will likely never be), in particular a comma (but also many others, eg. keywords.)
so, in the `select a /* one */ , /* two */ b from dual` example, i would fail to associate `/* one */` to `a` (and only to `a`) and/or `/* two */` to `b` (and only to `b`) without knowing that there was a comma between the two comments. Unfortunately, the AST does not contain the location of the comma (and/or many more keywords.) Another example would be: `SELECT ... GROUP /* here i am */ BY ...`
Unfortunately, i can't think of a good solution at the moment. (The litmus test being the reproduction of the original SQL's structure including its comments.)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3574115767
I see, thanks for clarifying! Yeah same, I can't think of a way for the parser to disambiguate such a scenario. But I think that could be fine that the decision be left up to the caller - in that since `find_between('a', 'b')` would be returning both `/* one */` and `/* two */`, the caller might have a preference on the more sensible strategy to use to associate the comments across both tokens
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3565797679
```rust
Parser::parse_sql_with_comments(..) -> Result<(Vec<Statement>, Comments), ..>)`
```
This approach sounds reasonable to me!
> that some tokens are omitted from the AST entirely, namely punctuation, making it difficult for a client to tell a comment's semantic position
@xitep do you mean that in the example the comments `/* one */` and `/* two */` aren't part of the tokenizer output to begin with? Offhand i would guess that doesn't have to be a special case in the tokenizer code
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3574557880
ok, thanks for your feedback @iffyio. i'll prepare a PR for that. even with the shortcuts we discussed, having `sqlparser-rs` publish the comments still seems a better idea to me than leaving clients completely without or with writing their own parser (essentially processing the sql source once again.)
btw: i'm more inclined to publish the comments' content as `&str` (as opposed to an owned `String`) with the lifetime bound to the parsed sql string. would you agree before i set off with the work?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio closed issue #2065: Limited Inline Comment Support in AST
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "RPG-Alex (via GitHub)" <gi...@apache.org>.
RPG-Alex commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3399916485
After looking around I have found [this](https://github.com/apache/datafusion-sqlparser-rs/blob/4490c8c55cbe1e6b96a29fd9ba448e06320af50a/src/tokenizer.rs#L459):
```rust
pub enum Whitespace {
Space,
Newline,
Tab,
SingleLineComment { comment: String, prefix: String },
MultiLineComment(String),
}
```
So not only does the tokenizer support comments it tracks the comment content!
I have then gone to the [parser mod](https://github.com/apache/datafusion-sqlparser-rs/blob/4490c8c55cbe1e6b96a29fd9ba448e06320af50a/src/parser/mod.rs#L454), where the `Tokenizer::new` is used, and after adding a `println!` on a small sql string to parse, I can see:
```
Tokens found: ... TokenWithSpan { token: Whitespace(SingleLineComment { comment: " this is a comment\n", prefix: "--" }), span: Span(Location(2,1)..Location(3,1)) }, ... }
```
(ellipses for omitted output for brevity)
For clarity the query I used:
```sql
-- this is a comment
SELECT * FROM foo; SELECT * FROM bar;
```
So it seems that the single line comment is being successfully tokenized! I may try to implement support for this depending on the difficulty around integrating with the parser.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3536296072
maybe a simpler and more general purpose idea for the parser to deal with comments could be the following one (feedback is highly be appreciated):
since comments can appear almost everywhere within a statement (or parsed script), either 1) the AST would have to provide a "before-/after-comment" for all tokens/nodes in the AST or 2) the parser would make up specific rules how to associated a single comment with a nearby token (this, i understood, is what is suggested as part of this issue.)
the former approach would mean very convenient access to the comments right away from the tokens themselves, but it would grow the AST (assuming that most tokens would not be associated with comments, it could be considered wasteful.) on the other, the parser making up its own mind about how to interpret and associate comments, it could be restrictive for downstream clients.
the other day, i wrote a project specific (java) code generator to process SQL like this ...
```sql
select id /* long */ ,
name /* String */ ,
...
```
... essentially associating metadata with particular tokens in the SQL query in a format of my choosing (for later code generation.)
it would be fully sufficient for my use-case(s) to have `sqlparser-rs` provide a `Parser::parse_sql_with_comments` method returning the AST as is, plus a separate, independent structure representing the extract comments, e.g.
```rust
Parser::parse_sql_with_comments(..) -> Result<(Vec<Statement>, Comments), ..>)`
```
with `Comments` (and associated) having the following API:
```rust
... Comments {
/// Possibly locates a SQL comment directly preceding the specified token
fn find_before(&self, token: &TokenWithSpan) -> Option<CommentWithSpan>;
/// Possibly locates a SQL comment directly following the specified token
fn find_after(&self, token: &TokenWithSpan) -> Option<CommentWithSpan>;
/// Retrieves all comments encountered.
fn as_slice(&self) -> &[CommentWithSpan];
}
struct CommentWithSpan {
span: Span,
text: Comment,
}
enum Comment {
SingleLine { content: String, prefix: String },
MultiLine(String),
}
```
I see the following advantages with this concept:
1. It does not require to extend existing AST nodes; since most of them will not be associated with comments at all this would save space
2. It would also mean that parsing _with_ and _without_ comments would not be a big difference; when parsing without comments we'd merely discard the comments (status-quo) instead of accumulating.
3. Since parsing and encountering comments is done from begin to end of the parsed SQL script (and comments not being overlapping), accumulating the comments (as encountering them while parsing) in a plain `Vec` would in fact end-up with a vec that can efficiently be searched with binary search by a given span's start/end.
what do you think?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3574947453
re representing the comment as `&str`, hmm that would be ideal but I'm not sure that wouldn't be quite invasive as things stand - since the tokenizer already produces the comments as owned `String`s - there's [ongoing work here](https://github.com/apache/datafusion-sqlparser-rs/issues/2036#issuecomment-3514033061) to do this end to end for the parser so maybe the comment handling can build on top of that work later on instead
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "RPG-Alex (via GitHub)" <gi...@apache.org>.
RPG-Alex commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3413485948
Connecting to PR #2069
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3742476100
hello, i would like to share an experiment i've been working on (it's merely an educational effort for me to learn more about parsing and rust in general); i've tried coding the idea outlined by @RPG-Alex above with a ["toy crate"](https://crates.io/crates/orql) and it seems to work. [this example](https://docs.rs/orql/0.1.0/orql/parser/index.html#parsing-sql-into-an-ast-with-token-locations-and-comments) demonstrates the use-case.
the crucial point is that `orql` exposes every token in its AST. but a fact is, that `orql` is merely a "toy." pushing it as far as `sqlparser` (limited to oracle sql), would be a _tons of work_ that i don't think is feasible for me. the question that i face now is whether i should invest my time into `orql` or `sqlparser`. would the concept of exposing every token in the AST be OK with `sqlparser`?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Limited Inline Comment Support in AST [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on issue #2065:
URL: https://github.com/apache/datafusion-sqlparser-rs/issues/2065#issuecomment-3575177497
alright, many thanks for the hint, @iffyio ! that sounds to me like at this point i should go with (owned) `String`s. Eventually it'd be reworked to a more efficient presentation. I believe it would be simply more consistent with the current crate API.
(btw: in my proof-of-concept, i keep comments internally as `Comment<Range<u32>>` with the range representing the start and endpoint in the parsed SQL. only when handing them out to the user i turn them into a `Comment<&str>` ... hence, the API proposal above.)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org