-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly look for end delimiter dollar quoted string #1650
Conversation
Currently the tokenizer throws an error for ```sql SELECT $abc$x$ab$abc$ ``` The logic is also quite difficult to read so I made it a bit simpler.
Adding some more tests first |
@iffyio let me know if see some improvements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall thanks @hansott! Left one comment regarding the suffix matching
src/tokenizer.rs
Outdated
temp.push(ch); | ||
|
||
if temp.ends_with(&end_delimiter) { | ||
s.push_str(&temp[..temp.len() - end_delimiter.len()]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I think indexing based on String::len
won't necessarily produce correct results on arbitrary UTF8 strings, we could something like strip_suffix
that doesn't rely on the byte count? e.g
if let Some(temp) = temp.strip_suffix(&end_delimiter) {
s.push_str(temp);
break
}
String::len won't necessarily produce correct results on arbitrary UTF8 strings
@iffyio Updated, thanks for your feedback! 🥇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the tokenizer throws an error for
SELECT $abc$x$ab$abc$
The logic is also quite difficult to read so I made it a bit simpler.
Before
After