Conversation
georgestagg
left a comment
georgestagg
left a comment
There was a problem hiding this comment.
Modulo clippy, and some picky comment edit requests, LGTM!
| // Split into FROM + non_from_sql_keyword so other_sql_statement can use | ||
| // just the non-FROM variant for its first token (preventing it from | ||
| // eating `FROM t VISUALISE ...` which should parse as from_statement). |
There was a problem hiding this comment.
I don't like it, but I see no other way of doing it other than explicitly declaring FROM as a conflict using the tree-sitter conflicts property, which is probably the worse of the two solutions.
Parsing our SQL parts of the query as just a bag of tokens is technically wrong, and we pay for it here (and in that nasty regex above). But the alternative-alternative is to build out the grammar properly and truly parse SQL queries (for all of our dialects), and that's just not going to happen in the short term.
There was a problem hiding this comment.
I really tried with all my might to make it work with a declared conflict to no avail. It was either this or preprocess the sql query with some even more nasty code
Co-authored-by: George Stagg <georgestagg@gmail.com>
| // F is excluded alongside W/S/C/I/U/D/V so the generic-word token can't | ||
| // absorb `FROM` — without this, tree-sitter tokenizes FROM as a plain | ||
| // word and other_sql_statement wins over from_statement. | ||
| const exclude_pattern = /[^\s;(),'"WwSsCcIiUuDdVvFf]+/; |
There was a problem hiding this comment.
Just remarking here that the grammar already gives all non-'other_sql_statement's precedence over other_sql_statement, so we wouldn't need the exclude_pattern.
I'm trying to throw it away in #364.
There was a problem hiding this comment.
It was def needed to get this to work but if you can do away with it then I'd be ecstatic
3 participants
This PR updates our grammar so that a preceding FROM is allowed in front of VISUALIZE.
This mirrors the syntactic sugar that DuckDB provides