fix: Handle double quotes inside quoted identifiers correctly #411
Conversation
This fixes apache#410 for standard SQL, however I don't know enough about other dialects to know if they handle this differently. May need more extensive testing as well.
Marwes
force-pushed
the
quoted_identifiers
branch
from
4ca1db4 to
113722e
Compare
Pull Request Test Coverage Report for Build 1807104062
💛 - Coveralls |
| #[test] | ||
| fn parse_quoted_identifier() { | ||
| pg_and_generic().verified_stmt(r#"SELECT "quoted "" ident""#); | ||
| } |
There was a problem hiding this comment.
This is definitely how postgres handles things
alamb=# SELECT "quoted "" ident" from t1;
ERROR: column "quoted " ident" does not exist
LINE 1: SELECT "quoted "" ident" from t1;
There was a problem hiding this comment.
However, msql treats this differently
Database changed
mysql> SELECT "quoted "" ident" from t1;
+----------------+
| quoted " ident |
+----------------+
| quoted " ident |
| quoted " ident |
| quoted " ident |
| quoted " ident |
| quoted " ident |
+----------------+
5 rows in set (0.00 sec)
mysql> SELECT "quoted ident" from t1;
+---------------+
| quoted ident |
+---------------+
| quoted ident |
| quoted ident |
| quoted ident |
| quoted ident |
| quoted ident |
+---------------+
5 rows in set (0.00 sec)
There was a problem hiding this comment.
" is used for strings in mysql unless ANSI_QUOTES is enabled https://dev.mysql.com/doc/refman/8.0/en/identifiers.html . Neither double quoted strings or ANSI_QUOTES seems implemented in this crate and I don't think it is up to this PR to fix that.
| @@ -834,6 +834,11 @@ fn parse_comments() { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn parse_quoted_identifier() { | |||
| pg_and_generic().verified_stmt(r#"SELECT "quoted "" ident""#); | |||
There was a problem hiding this comment.
perhaps we can add a test for dialects (e.g. mysql) that this isn't parsed as an identifier?
There was a problem hiding this comment.
I added a test for ` quoted strings in mysql
src/tokenizer.rs
Outdated
| @@ -418,8 +418,26 @@ impl<'a> Tokenizer<'a> { | |||
| quote_start if self.dialect.is_delimited_identifier_start(quote_start) => { | |||
| chars.next(); // consume the opening quote | |||
| let quote_end = Word::matching_end_quote(quote_start); | |||
| let s = peeking_take_while(chars, |ch| ch != quote_end); | |||
| if chars.next() == Some(quote_end) { | |||
| let mut last_char = None; | |||
There was a problem hiding this comment.
I think this code would be easier to understand if it were pulled out into its own function (peeking_take_while_ident?) which could be unit tested, to ensure it handles cases such as
foo "" bar ""
foo bar""
Added `pretty_assertions` so that the `assert_eq!` in the tokenization is readable
|
I hope to work through some various other PRs that are open this week and make a release later in the week |
Pull Request Test Coverage Report for Build 1806179475Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This fixes #410 for standard SQL, however I don't know enough about other dialects to know if they
handle this differently. May need more extensive testing as well.