Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,28 @@ 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('[') => {
let v = &self.value;
if v.len() >= 2 && v.starts_with('"') && v.ends_with('"') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// A nested double-quoted identifier (e.g. Redshift `["a]b"]`)
// keeps its inner quotes in the value and its `]` is already
// literal, so emit it unchanged.
write!(f, "[{v}]")
} else {
// Double a lone `]` so the identifier round-trips, but leave
// an already-doubled `]]` intact: in the tokenizer's no-escape
// mode the value still holds the raw `]]`, and re-doubling it
// would corrupt the identifier.
write!(f, "[")?;
let mut rest = v.as_str();
while let Some(pos) = rest.find(']') {
write!(f, "{}]]", &rest[..pos])?;
let after = &rest[pos + 1..];
rest = after.strip_prefix(']').unwrap_or(after);
}
write!(f, "{rest}]")
}
}
None => f.write_str(&self.value),
_ => panic!("unexpected quote style"),
}
Expand Down
33 changes: 33 additions & 0 deletions tests/sqlparser_mssql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,32 @@ fn parse_table_name_in_square_brackets() {
);
}

#[test]
fn parse_bracket_identifier_with_escaped_closing_bracket() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// A `]` inside a bracket-quoted identifier is escaped by doubling it, so
// `[a]]b]` denotes the identifier `a]b`.
let select = ms().verified_only_select("SELECT [a]]b]");
assert_eq!(
&Expr::Identifier(Ident::with_quote('[', "a]b")),
expr_from_projection(&select.projection[0]),
);

// Round-trips regardless of where the escaped `]` sits.
for sql in [
"SELECT [a]]b] FROM [c]]d]",
"SELECT []]]", // the identifier is a single `]`
"SELECT [a]]]", // trailing `]`
"SELECT []]b]", // leading `]`
"SELECT [a]]b]]c]", // several escaped `]`
] {
ms().verified_stmt(sql);
}

// In no-escape mode the parsed value keeps the raw `]]`, so serializing it
// must not double the brackets again.
ms_no_unescape().verified_stmt("SELECT [a]]b]");
}

#[test]
fn parse_for_clause() {
ms_and_generic().verified_stmt("SELECT a FROM t FOR JSON PATH");
Expand Down Expand Up @@ -2435,6 +2461,13 @@ fn ms() -> TestedDialects {
TestedDialects::new(vec![Box::new(MsSqlDialect {})])
}

fn ms_no_unescape() -> TestedDialects {
TestedDialects::new_with_options(
vec![Box::new(MsSqlDialect {})],
ParserOptions::new().with_unescape(false),
)
}

// MS SQL dialect with support for optional semi-colon statement delimiters
fn tsql() -> TestedDialects {
TestedDialects::new_with_options(
Expand Down