canonical / sqlair

Friendly type mapping for SQL databases
Apache License 2.0
17 stars 9 forks source link

Skip SQL comments #37

Closed Aflynn50 closed 1 year ago

Aflynn50 commented 1 year ago

This PR augments the parser to skip SQL comments as specified here https://www.sqlite.org/lang_comment.html.

Aflynn50 commented 1 year ago

This is unusual for the project, thus far, in the sense that it's changing the input buffer rather than parsing it through and skipping. It's not clear why it was done this way, since the logic required to know what to remove correctly is pretty much the same logic required to know what to skip correctly, except the former is more work as it's manipulating buffers instead of just walking through. Unless there's already some good rationale for doing it this way, I'd prefer to attempt an implementatino that does the same as everything else. A good analogy here is quoted strings: same idea, a start marker, and end marker, and stuff in the middle that must be skipped correctly.

I've found the reason I for this that I couldn't remember in our meeting! The issue is when a comment appears in the middle of something that we would parse as an output expression, e.g.:

SELECT
 col1, -- The first column
 col2, -- The second column
 col3, /* 
 possiblecol4 */
AS
 &Person.*
FROM
 table

In this case we have to worry about comments every time we have any gap between text. One alternative to this approach could be to worry about this in skipBlanks, and also parse them in a similar manner to string literals outside of the expressions. Would you prefer I try that approach?

niemeyer commented 1 year ago

As mentioned above, how would that be different from parsing a quoted string?

Aflynn50 commented 1 year ago

As mentioned above, how would that be different from parsing a quoted string?

I've changed it to skip the comments in place rather than a separate initial pass.

The main difference is that are not looking for quoted strings within input or output expressions. We now have two different entry points to removeComment, advance and skipBlanks.

Let me know what you think of this new way of doing it.