iffyio commented on code in PR #1620:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1620#discussion_r1898530317
##########
src/tokenizer.rs:
##########
@@ -1557,10 +1559,13 @@ impl<'a> Tokenizer<'a> {
};
} else {
value.push_str(&peeking_take_while(chars, |ch| {
- ch.is_alphanumeric() || ch == '_'
+ ch.is_alphanumeric()
+ || ch == '_'
+ || matches!(ch, '$' if
!self.dialect.supports_dollar_quoted_string())
}));
- if let Some('$') = chars.peek() {
+ let next_is_dollar = matches!(chars.peek(), Some('$'));
+ if next_is_dollar && self.dialect.supports_dollar_quoted_string() {
Review Comment:
```suggestion
if matches!(chars.peek(), Some('$')) &&
self.dialect.supports_dollar_quoted_string() {
```
##########
src/tokenizer.rs:
##########
@@ -1523,7 +1523,9 @@ impl<'a> Tokenizer<'a> {
chars.next();
- if let Some('$') = chars.peek() {
+ // Check if the second character is a dollar sign
+ let next_is_dollar = matches!(chars.peek(), Some('$'));
+ if next_is_dollar && self.dialect.supports_dollar_quoted_string() {
Review Comment:
```suggestion
// If the dialect does not support dollar-quoted string, then `$$`
is rather a placeholder.
if matches!(chars.peek(), Some('$')) &&
self.dialect.supports_dollar_quoted_string() {
```
if that makes sense, figured we could mention in the comment what the
condition implies?
##########
src/dialect/mod.rs:
##########
@@ -636,6 +636,12 @@ pub trait Dialect: Debug + Any {
false
}
+ /// Returns true if this dialect allows dollar quoted strings
+ /// e.g. `SELECT $$Hello, world!$$` or `SELECT $tag$Hello, world!$tag$`
+ fn supports_dollar_quoted_string(&self) -> bool {
Review Comment:
I wondering would it make more sense to flip the condition to e.g.
`supports_dollar_placeholder()`, setting that to true for sqlite and default
false for the others? Thinking that might be the more conservative change of
the two given that its the placeholder behavior that is more subtle/specific,
(most of the other dialects technically don't support dollar quoted string so
its less misleading if they don't have to explicitly flag otherwise)
##########
src/tokenizer.rs:
##########
@@ -2604,6 +2609,30 @@ mod tests {
);
}
+ #[test]
+ fn tokenize_dollar_placeholder_sqlite() {
Review Comment:
```suggestion
fn tokenize_dollar_placeholder() {
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
