Re: [PR] Escape closing brackets in bracket-quoted identifiers [datafusion-sqlparser-rs]

["LucaCappelletti94 (via GitHub)" <gi...@apache.org>]

LucaCappelletti94 commented on code in PR #2418:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2418#discussion_r3709902193


##########
tests/sqlparser_mssql.rs:
##########
@@ -937,6 +937,18 @@ fn parse_table_name_in_square_brackets() {
     );
 }
 
+#[test]
+fn parse_bracket_identifier_with_escaped_closing_bracket() {

Review Comment:
   Testing is insufficient and the proposed changes are currently regressing working cases. For instance, in unescaped mode, `SELECT [a]]b]` in current main parses correctly, while with this PR it parses to `a]]]]b`.



##########
src/ast/mod.rs:
##########
@@ -385,7 +385,19 @@ impl fmt::Display for Ident {
                 let escaped = value::escape_quoted_string(&self.value, q);
                 write!(f, "{q}{escaped}{q}")
             }
-            Some('[') => write!(f, "[{}]", self.value),
+            Some('[') => {
+                // Redshift nested quoted identifiers (e.g. `["a]b"]`) store the
+                // value as a complete double-quoted string whose inner `]` is
+                // literal, so leave those unchanged. Otherwise double each `]`,
+                // mirroring the tokenizer folding `]]` into `]`, so the
+                // identifier round-trips (#2409).

Review Comment:
   No need to refer to issues in the code itself. Code comments should refer to the present code, not previous states of the code, save for areas very prone to regressions and reiterated attempts.



##########
tests/sqlparser_mssql.rs:
##########
@@ -937,6 +937,18 @@ fn parse_table_name_in_square_brackets() {
     );
 }
 
+#[test]
+fn parse_bracket_identifier_with_escaped_closing_bracket() {
+    // A bracket-quoted identifier whose value contains `]` must serialize
+    // with the bracket doubled so it round-trips. See #2409.

Review Comment:
   Same here: comments should be about the code, not about previous states of the code. This is information appropriate for the PR post or commit, not code.



##########
src/ast/mod.rs:
##########
@@ -385,7 +385,19 @@ impl fmt::Display for Ident {
                 let escaped = value::escape_quoted_string(&self.value, q);
                 write!(f, "{q}{escaped}{q}")
             }
-            Some('[') => write!(f, "[{}]", self.value),
+            Some('[') => {
+                // Redshift nested quoted identifiers (e.g. `["a]b"]`) store the
+                // value as a complete double-quoted string whose inner `]` is
+                // literal, so leave those unchanged. Otherwise double each `]`,
+                // mirroring the tokenizer folding `]]` into `]`, so the
+                // identifier round-trips (#2409).
+                let v = &self.value;
+                if v.len() >= 2 && v.starts_with('"') && v.ends_with('"') {

Review Comment:
   I believe there are several other cases where the current solution fails, other than the one I reported in the test comment. I suggest you fuzz with seeding/use round trip prop tests this code before pushing the next iteration of your PR, since it would have most likely immediately caught the mentioned problems, even if you vibe code this thing using AI. It is a very effective support tool when you lean on code generation, as it gives you test inputs generation and invariant testing.



##########
src/ast/mod.rs:
##########
@@ -385,7 +385,19 @@ impl fmt::Display for Ident {
                 let escaped = value::escape_quoted_string(&self.value, q);
                 write!(f, "{q}{escaped}{q}")
             }
-            Some('[') => write!(f, "[{}]", self.value),
+            Some('[') => {
+                // Redshift nested quoted identifiers (e.g. `["a]b"]`) store the
+                // value as a complete double-quoted string whose inner `]` is
+                // literal, so leave those unchanged. Otherwise double each `]`,
+                // mirroring the tokenizer folding `]]` into `]`, so the
+                // identifier round-trips (#2409).
+                let v = &self.value;
+                if v.len() >= 2 && v.starts_with('"') && v.ends_with('"') {
+                    write!(f, "[{v}]")
+                } else {
+                    write!(f, "[{}]", v.replace(']', "]]"))

Review Comment:
   Here you are adding allocation in a hot path with that replace, in a write. I believe it is unnecessary to do so, just print what you need without reallocating the string.



-- 
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-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org