bombsimon commented on code in PR #1576:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870388122


##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,24 @@ impl Dialect for RedshiftSqlDialect {
     /// treating them as json path. If there is identifier then we assume
     /// there is no json path.
     fn is_proper_identifier_inside_quotes(&self, mut chars: 
Peekable<Chars<'_>>) -> bool {
+        // PartiQL (used as json path query language in Redshift) uses square 
bracket as
+        // a start character and a quote is a beginning of quoted identifier.
+        // Skipping analyzing token such as `"a"` and analyze only token that
+        // can be part of json path potentially.
+        // For ex., `[0]`, `['a']` (seems part of json path) or `["a"]` 
(normal quoted identifier)
+        if let Some(quote_start) = chars.peek() {
+            if *quote_start == '"' {
+                return true;
+            }
+        };

Review Comment:
   On `main`, an expression like `SELECT foo["bar"]` evaluates to a 
`JsonAccess` with the path containing an ident named `bar` but I guess that's 
wrong. What's a bit interesting though is that with this change, it will 
evaluate to an expression with alias (`SELECT foo AS ["bar"]`). I know this 
library doesn't care about what's actually valid SQL, but I wonder if it's an 
unwanted side effect? I guess it just drops the `[]` and parses `SELECT foo 
"bar"` which is short for `SELECT foo AS "bar"`.
   
   Another thing, is it intended to end up with `[]` as the quoting style and 
the value containing the quotes (`"a"`)?  Right now `["a"]` evaluates to an 
ident which I guess is what's causing above. Is `"a"` different from `a` as an 
identifier?
   
   ```rust
   Ident {
       value: "\"1\"",
       quote_style: Some(
           '[',
       ),
   },
   ```
   
   If we encounter a `"` as the first character in `[]` quotes, should we 
somehow verify what's quoted or ensure we have a closing `"`? If we don't, 
anything will go, e.g. `["=[]` would parse to an `Ident` with value `"[=` but 
maybe that's valid PartiQL?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to