Escape closing brackets in bracket-quoted identifiers - #2418
Conversation
A bracket-quoted identifier whose value contains ] (e.g. [a]]b], value a]b) serialized back to [a]b], which no longer re-parses. Double each ] on display, mirroring the tokenizer folding ]] into ]. Redshift nested quoted identifiers (["a]b"]) store the value as a complete double-quoted string whose inner ] is literal, so those are left unchanged. Fixes apache#2409 Signed-off-by: Haim Dimer <haim@dimer.org>
| // 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). |
There was a problem hiding this 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.
| #[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. |
There was a problem hiding this 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.
| } | ||
|
|
||
| #[test] | ||
| fn parse_bracket_identifier_with_escaped_closing_bracket() { |
There was a problem hiding this 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.
| if v.len() >= 2 && v.starts_with('"') && v.ends_with('"') { | ||
| write!(f, "[{v}]") | ||
| } else { | ||
| write!(f, "[{}]", v.replace(']', "]]")) |
There was a problem hiding this 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.
| // 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('"') { |
There was a problem hiding this 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.
2 participants
Fixes #2409.
A bracket-quoted identifier whose value contains
](e.g.[a]]b], valuea]b) serialized back to[a]b], which no longer re-parses. This doubles each]on display, mirroring the tokenizer which folds]]into].Redshift also parses nested quoted identifiers like
["a]b"], whose value is stored as a complete double-quoted string ("a]b") with a literal inner]. The AST doesn't distinguish that from a plain bracket ident, so a value that is a complete"..."string is left unchanged; otherwise the]are doubled.Added a round-trip test; the existing Redshift nested-identifier test covers the verbatim case.
Used AI assistance on this; I reviewed and tested it.