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